chore(e2e): stop and reap leaked isolated Obsidian E2E instances#1368
Merged
Conversation
Add stop:e2e-obsidian + reap-on-start (worktree-gone marker) + an orca.yaml archive hook so removed/merged worktrees no longer leak an Obsidian process tree or a /tmp profile dir. - scripts/stop-obsidian-e2e-instance.mjs: stop THIS worktree's instance (terminate its process tree found via the private --user-data-dir token, SIGTERM->SIGKILL, then remove its profile dir) + reapOrphanedInstances which reaps only instances whose backing worktree is GONE. Path guards refuse to rm anything that is not a direct child of the profile root matching the instance hash pattern. - start/cli: write a quickadd-e2e-instance.json marker recording the worktree, and reap stale instances on the next start/run (best-effort, never blocks). - orca.yaml archive hook runs the teardown on `orca worktree rm --run-hooks`. Targets only this worktree's instance; never touches the shared dev vault, other live worktrees, or PodNotes. Tooling only; no plugin behavior change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a complete Obsidian E2E instance teardown system: a new ChangesObsidian E2E Teardown System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying quickadd with
|
| Latest commit: |
2e1acfb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f295722f.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-e2e-autocleanup-or.quickadd.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/stop-obsidian-e2e-instance.test.ts (1)
351-353: ⚡ Quick winUse the shared marker constant instead of duplicating the filename literal.
Hardcoding
"quickadd-e2e-instance.json"here can drift from the producer/consumer contract. ReuseINSTANCE_MARKER_FILEto keep tests aligned automatically.♻️ Proposed refactor
+import { INSTANCE_MARKER_FILE } from "../scripts/start-obsidian-e2e-instance.mjs"; ... - path.join(spared, "quickadd-e2e-instance.json"), + path.join(spared, INSTANCE_MARKER_FILE), ... - path.join(leaked, "quickadd-e2e-instance.json"), + path.join(leaked, INSTANCE_MARKER_FILE),Also applies to: 362-364
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/stop-obsidian-e2e-instance.test.ts` around lines 351 - 353, The code uses a hardcoded filename string "quickadd-e2e-instance.json" instead of the shared constant INSTANCE_MARKER_FILE, which can cause drift between producer and consumer contracts. Replace the hardcoded string literal with the INSTANCE_MARKER_FILE constant in the path.join call at lines 351-353 and also at lines 362-364 where this same pattern appears, ensuring all references to the instance marker file use the same constant throughout the test file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/start-obsidian-e2e-instance.mjs`:
- Line 355: The reapStaleInstances call is being awaited at the startup point in
scripts/start-obsidian-e2e-instance.mjs, which blocks the entire startup process
and causes launch latency to grow with orphan count. Remove the await keyword
from the reapStaleInstances(options) call so it runs asynchronously in the
background without blocking startup. Additionally, check
scripts/obsidian-e2e-cli.mjs at line 173 for a similar await on reaping that
should also be converted to fire-and-forget behavior to ensure non-blocking
startup throughout the codebase.
---
Nitpick comments:
In `@tests/stop-obsidian-e2e-instance.test.ts`:
- Around line 351-353: The code uses a hardcoded filename string
"quickadd-e2e-instance.json" instead of the shared constant
INSTANCE_MARKER_FILE, which can cause drift between producer and consumer
contracts. Replace the hardcoded string literal with the INSTANCE_MARKER_FILE
constant in the path.join call at lines 351-353 and also at lines 362-364 where
this same pattern appears, ensuring all references to the instance marker file
use the same constant throughout the test file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b845ec2b-69cf-428f-a6c9-465bfc66a895
📒 Files selected for processing (8)
AGENTS.mdorca.yamlpackage.jsonscripts/obsidian-e2e-cli.mjsscripts/start-obsidian-e2e-instance.mjsscripts/stop-obsidian-e2e-instance.mjstests/start-obsidian-e2e-instance.test.tstests/stop-obsidian-e2e-instance.test.ts
Addresses CodeRabbit review on #1368: replace the hardcoded quickadd-e2e-instance.json literal with the shared INSTANCE_MARKER_FILE export so the test cannot drift from the producer/consumer contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Ports the isolated-Obsidian-E2E auto-cleanup feature that PodNotes shipped in
13ab5ebinto QuickAdd. Today, removing/merging a worktree leaves its isolated Obsidian instance running — leaking a whole Obsidian process tree plus a/tmp/quickadd-obsidian-e2e/<vault>-<hash>/profile dir. This adds three layers so that never happens, and a manual escape hatch.Tooling only — zero plugin behavior change (no
src/**touched).What changed
scripts/stop-obsidian-e2e-instance.mjs(new) +pnpm run stop:e2e-obsidian: stops this worktree's instance — finds the Obsidian process tree bound to its private--user-data-dirtoken (a per-worktree 12-hex hash), terminates it (SIGTERM → grace → SIGKILL for stragglers), and removes its profile dir. Flags:--dry-run,--prune(also reap orphans),--json. ExposesreapOrphanedInstanceswhich reaps only instances whose backing worktree is GONE.scripts/start-obsidian-e2e-instance.mjs+scripts/obsidian-e2e-cli.mjs: write aquickadd-e2e-instance.jsonmarker (recordsworktreePath) inprepareObsidianProfile, and reap-on-start — a best-effort self-healing pass that runs before launch/reuse and never blocks a start (lazyimport()keeps the module graph acyclic).orca.yaml(new):archivehook runs the teardown onorca worktree rm --run-hooks.tests/stop-obsidian-e2e-instance.test.ts(new, mirrors the reference; 23 cases) + a marker-producer assertion intests/start-obsidian-e2e-instance.test.ts. (Placed intests/per QuickAdd's existing convention for e2e-script tests.)AGENTS.md: documents the stop/reap workflow.Safety — only ever this worktree, only ever a gone worktree
--user-data-dir=<instancePath>/flag (trailing slash → no prefix bleed), in both/tmpand/private/tmp(macOS firmlink) forms. The shared dev vault (~/Library/Application Support/obsidian) isn't even under the profile root; sibling worktrees and PodNotes instances have different tokens → excluded.assertSafeInstancePathrequires an absolute path whose basename matches-[0-9a-f]{12}$and is a direct child of the profile root — refuses/tmp, relative paths, or anything outside the e2e tree.Verification (in this worktree's own isolated vault, hermetic private profile root)
Gates:
tsc✓ ·pnpm run lint✓ ·pnpm run check✓ (0 errors) ·pnpm test✓ 2394 passed / 31 skipped (incl. 23 new + marker-producer).End-to-end (real Obsidian, evidence captured):
/tmp, helpers on/private/tmp);stop:e2e-obsidiankilled the whole tree and removed the profile dir. Dev vault + 4 sibling-worktree instances untouched; shared root unchanged (16 dirs).startreaped only the orphan, spared the live one + the non-instance dir + my own instance.orca.yamlarchive command (cwd=worktree,$ORCA_WORKTREE_PATH=worktree); it invoked the stop script and tore down the running instance.Review
Reviewed by an ultracode multi-lens pass + 2 opposite-model (Codex) adversarial reviewers (Skeptic + Architect) → both CONTESTED, no high-severity. Accepted fixes: moved the test to
tests/(QuickAdd convention) and dropped a redundant vitest-include change; added the marker-producer test. Findings rejected as by-design parity with the reference (and documented): the pre-marker vault-path fallback, ps-snapshot PID-reuse TOCTOU, symlink path-vs-identity, matcher-not-name-scoped, the pre-existingstartalways-open -nbehavior, and the lazy-import seam.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests