fix(cli): stop lifecycle tunnel leaks and speed tickets reads#39
Conversation
|
![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: 378f7adc3f
ℹ️ 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".
| entry, | ||
| text, | ||
| size: metadata?.size ?? -1, | ||
| mtimeMs: metadata ? Math.trunc(metadata.mtimeMs) : -1, |
There was a problem hiding this comment.
Truncating metadata.mtimeMs to an integer can cause signature collisions when a journal file is rewritten with the same byte length and an unchanged (or restored) mtime, so readSnapshot may incorrectly accept a stale SQLite projection and return outdated ticket data. This regression is in the invalidation path introduced here: previously content changes were always detected because the file text was hashed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a70f1a2ac1
ℹ️ 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".
|
|
||
| const loadStoreContext = async (): Promise<TicketStoreContext> => { | ||
| const root = await git.ensureCheckedOut(); | ||
| const root = await git.ensureCheckedOut({ refreshRemote: false }); |
There was a problem hiding this comment.
loadStoreContext now always calls git.ensureCheckedOut({ refreshRemote: false }), which skips fetching remote refs even when origin is reachable. On a fresh clone (no local hack/tickets branch yet), ensureCheckedOut falls back to checkoutHead({ remoteUrl: null }) and creates an orphan local branch, so list/show/status read an empty snapshot despite existing remote tickets. This is a behavior regression from the previous default refresh path and causes false “no tickets found” / “ticket not found” results until a manual sync-like path runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 645e3702bd
ℹ️ 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".
| } | ||
| } else if (!fetched.missing) { | ||
| return { ok: false, error: `git fetch failed: ${fetched.error}` }; | ||
| if (input.allowRemoteFetchFailureFallback) { |
There was a problem hiding this comment.
Allowing fetch-failure fallback here can permanently poison a fresh tickets clone: with no local tickets branch yet, a transient remote error falls through to orphan branch initialization, and subsequent read paths (store.loadStoreContext now calls ensureCheckedOut({ refreshRemote: false })) keep reusing that empty local branch without refetching even after connectivity is restored. Fresh evidence: forcing the first listTickets() to fail against an unreachable origin, then fixing origin, still returns an empty ticket list because the local orphan branch already exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1f728b8a2
ℹ️ 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".
| for (const processInfo of opts.lifecycleEntry?.processes ?? []) { | ||
| if (processInfo.panePid) { | ||
| rootPids.add(processInfo.panePid); | ||
| } | ||
| if (processInfo.processGroupId) { | ||
| processGroupIds.add(processInfo.processGroupId); | ||
| } |
There was a problem hiding this comment.
resolveLifecycleProcessGroupIds unconditionally seeds rootPids and processGroupIds from the persisted lifecycle state, then terminateLifecycleProcessGroups sends signals to every collected group. If a lifecycle pane already exited before shutdown and its PID/PGID has been recycled, this path can target unrelated host processes and kill them during down. Only IDs proven to still belong to the current tmux session/window should be included in the kill set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a21672fdd
ℹ️ 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".
|
|
||
| const loadStoreContext = async (): Promise<TicketStoreContext> => { | ||
| const root = await git.ensureCheckedOut(); | ||
| const root = await git.ensureCheckedOut({ refreshRemote: false }); |
There was a problem hiding this comment.
loadStoreContext now calls git.ensureCheckedOut({ refreshRemote: false }), which reuses an existing local tickets branch without fetching origin updates; in a multi-clone workflow, if another clone creates a ticket after this clone’s last fetch, setStatus/update paths that call materializeTickets() will read stale state and return Ticket not found for a real remote ticket. This is a regression from the previous ensureCheckedOut() behavior and breaks normal status/update operations unless users manually run an explicit sync first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40fd6f22ff
ℹ️ 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".
| "cleanup_lifecycle() {", | ||
| " trap - EXIT INT TERM HUP", | ||
| ` if [ -n "\${cmd_pid:-}" ]; then`, | ||
| ' kill -TERM -- "-$cmd_pid" 2>/dev/null || kill "$cmd_pid" 2>/dev/null || true', |
There was a problem hiding this comment.
wrapLifecyclePersistentCommand is executed via sh -c, and on common Linux setups (/bin/sh = dash) the builtin kill does not accept the -- separator, so kill -TERM -- "-$cmd_pid" fails every time and falls through to kill "$cmd_pid". That only signals the leader process, so lifecycle commands that spawn child tunnel/proxy processes can still leak children after down or signal handling on Linux.
Useful? React with 👍 / 👎.
## 2.2.0 (2026-04-01) * Merge branch 'main' into dimitri/exec-command-surface ([e2eb7f6](e2eb7f6)) * Merge branch 'main' into dimitri/exec-command-surface ([a068e7e](a068e7e)) * Merge branch 'main' into dimitri/fix-lifecycle-and-tickets-speed ([0ce84f7](0ce84f7)) * Merge branch 'main' into dimitri/fix-lifecycle-and-tickets-speed ([7a21672](7a21672)) * Merge pull request #38 from hack-dance/dimitri/exec-command-surface ([56c2e9e](56c2e9e)), closes [#38](#38) * Merge pull request #39 from hack-dance/dimitri/fix-lifecycle-and-tickets-speed ([2ad76b4](2ad76b4)), closes [#39](#39) * fix(cli): harden lifecycle cleanup and isolate exec tests ([18904e0](18904e0)) * fix(cli): harden lifecycle cleanup and local tickets reads ([a70f1a2](a70f1a2)) * fix(cli): stop lifecycle tunnel leaks and speed tickets reads ([378f7ad](378f7ad)) * fix(cli): tighten lifecycle shutdown and ticket refresh ([74bd742](74bd742)) * fix(runtime): harden exec session checks ([9ee1d8d](9ee1d8d)) * fix(tickets): fail fresh clone reads on remote fetch errors ([e1f728b](e1f728b)) * fix(tickets): preserve projection freshness on reads ([645e370](645e370)) * test(runtime): isolate exec command logger behavior ([382a84c](382a84c)) * test(tickets): cover unreachable remote checkout recovery ([40fd6f2](40fd6f2)) * feat(runtime): add exec command for live service containers ([e6a3a9e](e6a3a9e))
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18904e04aa
ℹ️ 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".
| ' python3 -c \'import os, sys; os.setsid(); os.execvp("sh", ["sh", "-lc", sys.argv[1]])\' "$HACK_LIFECYCLE_COMMAND" >"$fifo" 2>&1 &', | ||
| "else", | ||
| ' sh -lc "$HACK_LIFECYCLE_COMMAND" >"$fifo" 2>&1 &', |
There was a problem hiding this comment.
The new wrapper executes lifecycle processes with sh -lc in both branches, which turns sh into a login shell and causes profile files to be sourced before the command runs. On Linux systems where /bin/sh is dash, non-POSIX content in ~/.profile can make the shell exit before the lifecycle command starts (I reproduced this with /bin/sh -lc 'echo ok' exiting with profile errors), so lifecycle hooks/tunnels fail to launch. The previous implementation used non-login execution and did not have this startup dependency.
Useful? React with 👍 / 👎.
1 participant
Summary
C-cbefore killing the lifecycle session so host-side proxy scripts can clean up child tunnel processesVerification
bun test tests/tickets-store.test.ts tests/project-restart-command.test.tsbun run typecheckbun x ultracite check src/commands/project.ts src/control-plane/extensions/tickets/sqlite-projection.ts tests/tickets-store.test.ts/Users/hack/dev/external/TeamSidewinder/event-agentbun run index.ts down --path /Users/hack/dev/external/TeamSidewinder/event-agentevent-agent--lifecycletmux session was removed127.0.0.1:{3306,9200,9201,8443,8444,8445}were cleared after shutdownFollow-up
T-G1TRWBHXY4Follow up lifecycle teardown for branch and non-tmux sessions