fix(machine): route Machine DELETE through canonical page services + scrub agent refs#2043
Conversation
…scrub agent refs Machine DELETE previously trashed its page via raw pageRepository.trash, bypassing the canonical page-trash path: no descendant cascade-trash, no revision bump / page version, no page-trash workflow triggers, and the deleted machineId lingered as a dangling ref in AI_CHAT agents' machines jsonb arrays (GA blocker, deferred items 4+5 of the Machine/Terminal arc). - MachineSettingsStore.trashPage now routes through pageService.trashPage (trashChildren: true) — cascade, revision/version, and trash triggers all happen exactly as on the page DELETE route; the trashed broadcast keeps the same payload shape. - New MachineRefScrub seam in deleteMachine: after the trash and before the Sprite teardown, scrub the machineId from every referencing AI_CHAT agent's machines array (via canonical applyPageMutation, operation agent_config_update, changeGroupType system) and from global_assistant_config.machines (the same pair migration 0195 rewrote). Best-effort like the teardown — reported as agentRefsScrubbed, never fails the delete, never skips the Sprite kill. - PR #1967's ordering/reversibility invariants preserved: trash FIRST (reversible), remote teardown LAST (host errors land in the recoverable path), dependent machine_* metadata rows still left in place for restore. Tests: pure-module ordering + failure isolation; runtime canonical-trash routing, broadcast, ref filtering (incl. malformed-sibling preservation), sweep-on-partial-failure, global-config rewrite; route contract updates. typecheck green (packages/lib + apps/web). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMmatEURu4tTTrdjP8yYQR
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb6538ccf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // route's canDeleteMachine gate is the same check, so this never flips | ||
| // an authorized delete to a failure. | ||
| const result = await pageService.trashPage(machineId, actorUserId, { | ||
| trashChildren: true, |
There was a problem hiding this comment.
With trashChildren: true, deleting a Machine now trashes every descendant page, but the later Sprite teardown still runs only for the top-level machineId. I checked pageService.createPage and it only verifies that a parent page exists, not that the parent is a folder, so a descendant can also be a MACHINE; in that case the child Machine page is hidden by this cascade while its own session and branch Sprites are never killed. Please either discover/teardown descendant Machine pages as part of this delete or avoid cascading Machine subtrees.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a2f19c2. createMachineSpriteTeardown().teardown now tears down the whole trashed subtree: collectDescendantMachineIds BFS-walks parentId from the deleted root (post-trash, so deliberately no isTrashed filter) and collects every MACHINE-typed descendant; each is torn down via the extracted teardownOneMachine (branch Sprites best-effort, then own Sprite), descendants first, root last. One machine failing never strands the rest — failures are counted and rethrown at the end so deleteMachine still reports spriteTornDown: false. Covered by new tests: nested-machine teardown order, descendant-failure isolation, and the no-op case.
| await applyPageMutation({ | ||
| pageId: agent.id, | ||
| operation: 'agent_config_update', | ||
| updates: { machines: filtered }, |
There was a problem hiding this comment.
When the deleted Machine is the only configured ref for an agent, filtered becomes [] while machineAccess remains enabled. I checked resolveConfiguredMachines in apps/web/src/lib/ai/tools/sandbox-tools-runtime.ts, and page agents with machineAccess on and an empty machines array fall back to { kind: 'own' }, so deleting a shared Machine silently switches that agent to a different Machine instead of leaving it without the removed configuration. Please disable machine access or preserve an explicit empty selection when the last ref is scrubbed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a2f19c2. Confirmed against resolveConfiguredMachines — machineAccess: true + machines: [] does fall back to {kind:'own'}. The scrub now selects machineAccess alongside the refs and, when filtering empties the list for an agent that had access on, the same applyPageMutation write sets machineAccess: false (updates + updatedFields both carry it, single canonical mutation). The agent's owner re-enables and re-links explicitly. Tests: last-ref scrub disables access; remaining-refs scrub leaves machineAccess untouched.
| .update(globalAssistantConfig) | ||
| .set({ | ||
| machines: sql`( | ||
| SELECT coalesce(jsonb_agg(elem), '[]'::jsonb) |
There was a problem hiding this comment.
For a global assistant config whose only entry is the deleted Machine, this rewrite stores [] but leaves machineAccess true. I checked resolveGlobalConfiguredMachines in apps/web/src/lib/ai/tools/sandbox-tools-runtime.ts, and that exact state falls back to { kind: 'own' } and can provision/use the user's personal Machine, so removing an existing Machine unexpectedly changes the assistant's active machine rather than clearing access. Please update machineAccess (or otherwise distinguish explicit empty config) when the scrub removes the final ref.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a2f19c2. The global_assistant_config UPDATE now also sets machineAccess via a CASE that flips it to false when no elements survive the filter (NOT EXISTS (SELECT 1 FROM jsonb_array_elements(machines) elem WHERE NOT (elem @> ref))). Both SET expressions read the OLD row in the same statement, so the rewrite and the emptiness check see the same pre-update list — no window where access stays on over an emptied config, and no fallback to auto-provisioning the user's personal Machine via resolveGlobalConfiguredMachines.
…rub empties refs Review fixes (PR #2043, 3 threads): - Teardown now covers the whole trashed subtree: nothing prevents nesting a MACHINE page under another, and the new cascade-trash hides nested Machines whose Sprites were never killed. collectDescendantMachineIds walks the tree (post-trash, so no isTrashed filter) and every Machine is torn down descendants-first; one failure never strands the rest, and any own-Sprite kill failure still surfaces as spriteTornDown=false. - Scrubbing the LAST ref now disables machineAccess (agent pages and global_assistant_config alike): resolveConfiguredMachines / resolveGlobalConfiguredMachines treat access=true + machines=[] as a fallback to {kind:'own'}, so an emptied list would have silently repointed the agent at a different machine (and auto-provisioned a personal Machine on the global path). The owner re-enables and re-links explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMmatEURu4tTTrdjP8yYQR
Page moves reject cycles, but a corrupt tree must degrade to a bounded walk, not an infinite teardown loop — visited-set guard + test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMmatEURu4tTTrdjP8yYQR
1 participant
Summary
GA-blocker remediation for deferred items 4 + 5 of the Machine/Terminal arc (page
wm76dkca6cd2jwcldumys5c5, taskcj9bdw5wj4yfgbje2uuflzfu): Machine DELETE previously soft-deleted its page via rawpageRepository.trash, bypassing the canonical page services. That meant no descendant cascade-trash, no revision bump / page version, no page-trash workflow triggers, and the deletedmachineIdwas left dangling in AI_CHAT agents'machinesjsonb arrays.What changed
1. Canonical page-trash —
createDbMachineSettingsStore().trashPagenow routes throughpageService.trashPage(machineId, actorUserId, { trashChildren: true }), exactly what the page DELETE route (/api/pages/[pageId]) does: descendant cascade,applyPageMutationrevision bump + page version, and deferred page-trash workflow triggers. Thetrashedbroadcast keeps the canonical payload shape (title+parentId). A trash failure still throws (the non-recoverable first step), sodeleteMachinenever proceeds to teardown on a failed trash.2. Delete-time ref scrub — new
MachineRefScrubseam indeleteMachine, wired bycreateDbMachineRefScrub(actorUserId):machinesarray contains{kind:'existing', machineId}gets that entry removed via the canonicalapplyPageMutation(operation: 'agent_config_update',changeGroupType: 'system', per-agentexpectedRevision) — same treatment aspageAgentRepository.updateAgentConfig. Only the matching element is removed; all other entries (including malformed ones) are preserved byte-for-byte. Per-agent failures don't stop the sweep; the failure is thrown at the end so the result reports it.global_assistant_config.machinesis rewritten too — the same MachineRef pair migration 0195 covered in the rename backfill; this is the delete-time counterpart.3. PR #1967 ordering/reversibility preserved —
deleteMachineorder is now trash (reversible, FIRST) → ref scrub → Sprite teardown (remote, LAST, so host errors land in the recoverable path). Both post-trash steps are independently best-effort with their own result flags (agentRefsScrubbedjoinsspriteTornDown) — a scrub failure never skips the Sprite kill (no leaked microVMs) and vice versa. Branch-Sprites-then-machine-Sprite teardown order and the "dependentmachine_*metadata rows stay for restore" invariant are untouched.Review fixes (a2f19c2)
isTrashedfilter — it runs post-trash) and tears down every Machine in it, descendants first, root last; one failure never strands the rest, and any own-Sprite kill failure still reportsspriteTornDown: false.machineAccesswhen the scrub empties the refs —resolveConfiguredMachines/resolveGlobalConfiguredMachinestreatmachineAccess: true+machines: []as a fallback to{kind:'own'}(the global path even auto-provisions a personal Machine), so removing an agent's last ref would have silently repointed it at a different machine. The agent write carriesmachineAccess: falsein the same canonical mutation; the global-config UPDATE flips it via a CASE in the same statement.collectDescendantMachineIds's subtree walk now tracks visited ids. Page moves reject cycles today, but a corrupt tree must degrade to a bounded walk, not an infinite teardown loop.master(two unrelated sibling fixes — cron scheduling, Canvas CSP nonce — no file overlap with this PR's changes); no conflicts.Out of scope (unchanged, documented in the module doc)
updateSettings) still writes settings fields via rawdb.update— canonicalizing the rename/toggle path is a separate follow-up.machine_agent_terminals/machine_projects/machine_branchesrows are deliberately left in place (FK-cascade on hard purge; kept for restore — per PR feat(machines): Machine Settings CRUD route (GET/PATCH/DELETE) #1967's reverted-hard-delete lesson).Tests
packages/lib): trash→scrub→teardown ordering, scrub failure isolation (delete still succeeds, teardown still runs), teardown failure isolation, trash failure propagation, not_found short-circuit — 9 pass.apps/web/src/lib/machines/__tests__/machine-settings-runtime.test.ts): canonicalpageService.trashPagerouting withtrashChildren: true(the cascade + revision + trigger behavior is that service's own tested contract), trashed broadcast parity, failure propagation without broadcast, no hard deletes (restore-compatible), ref filtering incl. malformed-sibling preservation, last-ref scrub disablingmachineAccess(and leaving it alone when refs remain), sweep-on-partial-failure then throw, global-config rewrite incl. the access CASE, nested-machine subtree teardown order, descendant-failure isolation, parentId-cycle termination, teardown no-op — 16 pass.refsdep wired,agentRefsScrubbedin response + audit — 21 pass.tsc --noEmitgreen inpackages/libandapps/web.🤖 Generated with Claude Code
https://claude.ai/code/session_01HMmatEURu4tTTrdjP8yYQR