Repository navigation
fix(git): bound team-repo pull, fetch and push with the block timeout and end the git process tree - #1777
Conversation
… and end the git process tree pullRepo, pullRepoFastForward, the push in pushRepoDirectly and pushRepoBranch, pushRepoBranch's fetch, the team-ref fetch and fetchReportsOriginFile now run on createGitWithBlockTimeout, with --progress so a transfer that still moves keeps resetting the timeout. When the block timeout or the caller's abort fires, the factory ends git and every process under it (ps + SIGTERM, taskkill /T /F on Windows), so a git fetch a pull started and the ssh under it no longer survive and hold the CLI open. A stalled command fails with GitStalledError naming it, and pullRepo no longer retries a stalled pull as a fetch + reset. Fixes Tencent#1749
|
No confident findings in the requested diff. The PR description documents sufficient testing: build, lint, unit tests, and representative real-CLI runs at Review was read-only; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving 0f726f70. It fixes #1749. We built main (72a492ac, also the merge-base) and this head, and used a stand-in ssh command that serves a local repo and can be made to go silent:
| case | main | this head |
|---|---|---|
pull, silent remote |
hangs; git pull, git fetch and ssh stay alive after node is killed |
exit 1 after about 5.5 s, names git pull --ff-only --progress |
pull, default timeout |
hangs | exit 1 after 31 s |
push, silent at the push step |
hangs | exit 1 after 5.7 s, names git push --progress … |
members list, silent reports fetch |
hangs | exit 0 from the local copy |
| slow but moving transfer (about 6 MB over 26 s, 3 s timeout) | pull and push succeed | pull and push succeed |
After every stalled run on this head, no git or ssh process was left 2 s later and there was no .lock file. TEAMAI_INIT_PUSH_TIMEOUT_MS, which the error names, works: against a remote silent for 7 s, 5000 fails and 15000 succeeds. From the code, endProcessTree signals only descendants of the git child. The diagnostics-channel lookup cannot pick another child when several git calls run at once. The listener is registered once, and POSIX uses SIGTERM, so git removes its locks.
Non-blocking:
- [P2 non-blocking] The new
pscall atsrc/utils/git.ts:396has nowindowsHide: true. #1775 adds a lint rule that requires it on every child-process launch, so whichever of the two lands second failsnpm run lintuntil that option is added. It is one line. - [P3 nit] A git hook, or a password or host-key prompt, that stays silent for the whole timeout is now ended. The body discloses this, and the environment variable raises the limit.
The CI review (#1777 (comment)) reports no findings. We agree. The interaction with #1775 is outside this diff.
…n place on a failed e2e setup Tencent#1777 added a spawnSync('ps') without windowsHide, which the new teamai/windows-hide rule rejects once this branch sits on main. Case (b) of the Windows lifetime e2e moved ~/.teamai aside before its setup, outside the try/finally that restores it. Move it aside last.
The status fetch, the promotion fetch and push, and remoteBranchExists's ls-remote still ran on plain createGit after #1777. A stalled ssh origin held teamai status, the sync lock around recall promote, and the pending push prune forever. Bound them with createGitWithBlockTimeout and --progress; local status/add/commit stay on the plain instance.
…n place on a failed e2e setup Tencent#1777 added a spawnSync('ps') without windowsHide, which the new teamai/windows-hide rule rejects once this branch sits on main. Case (b) of the Windows lifetime e2e moved ~/.teamai aside before its setup, outside the try/finally that restores it. Move it aside last.
Summary
A team repo whose
ssh://remote stalls after the channel opens hungteamai pull,teamai push's pre-pull and the SessionStart pass forever, and leftgit pull→git fetch→sshrunning (#1749). As the triage and the maintainer decision set out, the team-repo pull, fetch and push now run on the existingcreateGitWithBlockTimeout/gitBlockTimeoutMsseam with--progress, the factory ends git's whole process tree when the timeout or the caller's abort fires, and the error names the git command that stalled.Type of Change
Change
src/utils/git.ts:createGitWithBlockTimeout(basePath?, abort?)takes the optionalabortthatcreateGittakes, sopullRepoandsrc/team-ref.tskeep the git-hook pull's 5 s cap.kill. The factory wraps thatkillto call the newendProcessTree(pid)first:ps -A -o pid=,ppid=before signalling anything, because a killed parent's children move to init and can no longer be found. It then sends SIGTERM to each process. git handles SIGTERM by removing its lock files, and ssh exits on it.taskkill /PID <pid> /T /F.pscannot list the tree, only git is signalled, as before.child_processdiagnostics channel. The handler takes the child whosestdoutis the stream simple-git just passed in. Both happen in the same tick as the spawn.GitStalledError, for examplegit pull --ff-only --progress stalled: no output for 30s, so teamai ended it and the processes it started. Set TEAMAI_INIT_PUSH_TIMEOUT_MS (milliseconds) to wait longer.It extends simple-git'sGitError, which simple-git rethrows unchanged. Any other error class would come back wrapped in a plainGitErrorthat keeps only its text. An abort keeps simple-git's own error, whichpull.tsalready recognises throughcap.aborted.--progress:pullRepo: the pull and its fetch for the fetch + reset recovery.pullRepoFastForward.pushRepoDirectly.pushRepoBranch.pullReporethrows aGitStalledErrorwithout trying the fetch + reset recovery, since a fetch from the same remote would stall again and double the wait.createGitunlessinitPushalready put them under the timeout.gitBlockTimeoutMsnow says what the timeout covers: remote calls, run with--progress. Local commands of the team repo stay unbounded.src/team-ref.ts: thefetch --pruneof branches and tags uses the block-timeout factory with the hook's signal.src/utils/reports-branch.ts:fetchReportsOriginFile'sls-remoteand fetch use it too.autoPushViaMRis untouched (#1760 owns it). It reaches the bound throughpushRepoBranch.Test Plan
npx tsc --noEmitpasses (npm run typecheck)npm run lintpassesnpx vitest runpasses: 738 files pass.dashboard-collector,env-loader(zsh cases) andtoken-usagefail with 8 tests, identically on unmodifiedmainon this macOS machine, and are unrelated. In an earlier full run, oneworkflow-skill-drivertest failed once and passed when rerun alone.src/__tests__/git-stalled-remote.test.ts(POSIX) uses real git against anssh://origin through acore.sshCommandstand-in. While the stand-in serves,pullRepostill reports1 file(s) changed, so--progressdoes not break the summary. Once it accepts and never writes (and ignores SIGINT), three cases run:pullReporejects with aGitStalledErrornaminggit pull --ff-only --progressin one timeout.pullReporejects.fetchTrackingRefrejects naming its fetch.Each of the three then checks that the stand-in and the git that started it are gone. On
mainall three fail: the pull and the side-branch fetch time out at 10 s, and the abort leavesgit fetchand the stand-in alive (expected [ 97237, 97236 ] to deeply equal []).src/__tests__/git-process-tree.test.tschecks thatendProcessTreeends a node → node → node tree. It is added toWINDOWS_TESTS, so the Windows CI job runs thetaskkillbranch. Onmainit fails (endProcessTree is not a function).git-kind-reports.test.ts: the stalled-origin write now has to namegit push --progress origin teamai-reports … stalled.git.test.tsexpects the new--progressarguments.git.test.tsandinit.test.tskeep simple-git's real exports in their mock, becauseGitStalledErrorextendsGitError.Real-CLI record, after
npm run build. Setup:env -i, XDG dirs inside it) and a workspace initialized withteamai init ssh://git@stall.invalid/<sandbox>/team.git --provider git.core.sshCommandis a/bin/shstand-in withssh.variant=simple. It serves the local bare repo by running the requestedgit-upload-pack/git-receive-pack. Once a flag file exists, it accepts, never writes, and ignores stdin EOF and SIGINT. No network host is contacted.ps -A -Elists every process that carries the sandbox HOME or path, plus its children.TEAMAI_INIT_PUSH_TIMEOUT_MS=5000lowers the timeout. The last run uses the default 30 s.members listreads theteamai-reportsside branch through the reports worktree refresh.mainat 72a492a:this head (0f726f7):
members listtakes two timeouts on this head: thels-remotethat asks whether the branch is on origin, which is already on the block-timeout factory and swallowed as "unknown", and then the fetch named above.Related Issues
Fixes #1749
Notes for Reviewers
Not verified on Windows. I have no Windows machine.
taskkill /PID <pid> /T /Fis untested by me.git-process-tree.test.tsis inWINDOWS_TESTS, so the Windows CI job will run it against a node process tree, but not against git or Windows OpenSSH. The reporter's actual stall (Windows OpenSSH 9.5p2 in session 0) was not reproduced, only its symptom. HTTPS stalls and the SessionStart background pass through a real agent were not rerun either. The SessionStart pass goes through the sametransitionTeamRepo→pullRepopath asteamai pull.Why the diagnostics channel. simple-git 4 has no public hook that exposes the spawned process, and
spawnOptionsaccepts onlyuid/gid, so git cannot be started in its own process group. The alternative was reaching into simple-git's private plugin store, which a minor release could break. Node's built-inchild_processchannel is documented. It is marked experimental among the built-in channels. I checked it publishes on Node 20.10 and 24.14. The match is by stdout identity, so a child from any other spawn is never taken for git. If the channel ever stops publishing, the factory falls back to the old behaviour (git alone is signalled).SIGTERM, not SIGKILL, on POSIX. A tree member that ignores SIGTERM would survive, but git cleans its lock files on SIGTERM and would not on SIGKILL. That matters for the abort, which can land while the pull is writing. Windows has no equivalent:
/Fis required for console processes, and simple-git already usedTerminateProcessthere for git itself.Why the e2e and test stand-in ignores SIGINT. On macOS,
git fetchforwards the SIGINT that simple-git sends it to its ssh and waits for it. So with an ssh that exits on SIGINT, a plain side-branch fetch already ended onmain, and only thegit pull→git fetchchain and the abort were left behind. The stand-in ignores SIGINT to stand for an ssh that git cannot interrupt. Theteamai pullresult onmainis the same either way.Residual risk from the maintainer's choice to bound pull and push. A team repo hook that runs inside the bounded command can now be ended if it prints nothing for the whole timeout:
post-mergehook duringgit pull;pre-pushhook duringgit push;Hook output resets the timer, and
TEAMAI_INIT_PUSH_TIMEOUT_MSraises it. The env var keeps its name, although it now covers more than init pushes.Remote calls left on
createGit, to keep this to the calls the triage listed:remoteBranchExists,pushLearningToOriginandwithDefaultBranchPreview's fetches insrc/utils/git.tsandreports-branch.ts;withKnowledgeWorktree;session-flow/archive.ts,workflow/cache.ts, andmaintenance/promote.ts/promotion-publish.ts.Each could move the same way in a follow-up.
The optional
teamai doctorwarning is not part of this PR. No doc or skill text describes these timeouts.docs/,skill-data/and the READMEs were grepped forTEAMAI_INIT_PUSH_TIMEOUT_MS, block timeout andlowSpeed.