Skip to content

fix(hooks): drain usage reports before background exit - #1324

Closed
ydflow wants to merge 1 commit into
Tencent:mainfrom
ydflow:fix/hook-drain-usage-report
Closed

ydflow wants to merge 1 commit into
Tencent:mainfrom
ydflow:fix/hook-drain-usage-report

Conversation

@ydflow

@ydflow ydflow commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Summary

A SessionStart pull stops awaiting its usage report after five seconds, but the detached hook-dispatch --bg-only process then calls process.exit(0). A slow report can be terminated after committing locally and before publication, acknowledgement, or lock cleanup. This leaves reports accumulating locally and usage unacknowledged (#1322).

Drain the pending usage report before the detached hook exits, with a 60-second bound. Track its sync-lock cleanup in the pending promise too, so draining cannot finish between acknowledgement and lock release. The foreground hook retains its existing exit behavior and seven-second safety cap; the ordinary pull's five-second wait is unchanged. Add focused regressions and bilingual hook documentation.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Test Plan

  • npm run build
  • npx tsc --noEmit
  • npm run lint
  • git diff --check
  • npx vitest run src/__tests__/pull-scope-isolation.test.ts -t 'usage|report|recall log' --maxWorkers=1 — 15 passed, including the two new drain/cleanup/deadline regressions.
  • npx vitest run src/__tests__/async-timeout.test.ts src/__tests__/team-push-privacy.test.ts src/__tests__/team-push-interventions.test.ts src/__tests__/reports-branch-readonly.test.ts --maxWorkers=2 — 37 passed.
  • npx vitest run --config vitest.windows.config.ts --maxWorkers=3 — 665 passed.
  • Added/updated tests for the change.

Built-CLI verification

Built dist/index.js, then ran these without retries:

npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/report-timeout.test.ts -t 'session-start' --maxWorkers=1 --retry=0
npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/report-timeout.test.ts -t 'git/claude|retains a rejected report' --maxWorkers=1 --retry=0

Both commands passed (2 + 2 cases). These launch real CLI subprocesses against a local bare Git remote. Its reports update hook sleeps seven seconds, exceeding the original five-second wait. The new cases invoke hook-dispatch session-start --tool claude --bg-only: successful pushes reach the remote, acknowledge usage and session snapshots, release both .reports-lock and .sync-lock, and do not count again on the next pull. A delayed rejection logs the push failure, retains usage, releases locks, and retries successfully once the remote accepts writes.

Before the fix, the successful-background-hook case failed: dispatch exited after Auto-report is still running after 5s, before the remote had a stats file. The existing direct-pull success/rejection cases still pass.

Local validation limits

Windows / Node 24. The full pull-scope-isolation.test.ts run had 31 pre-existing failures (19 passed with this change; pristine base 2401d04 had 17 passed and the same 31 failure names). The expanded hook dispatcher/handler run had 180 passed and 6 failures; all 6 failing cases also fail on pristine 2401d04. These involve existing mock exports, shell/environment behavior and background-spawn tests. This is not a claim that the complete unit suite passes locally. The full POSIX suite and additional provider/agent combinations are left to CI; no remote service or model credentials were used.

Related Issues

Fixes #1322

Notes for Reviewers

The 60-second drain is bounded and only runs in the detached child. If the deadline expires, it logs a warning and leaves unacknowledged usage for retry; it does not claim successful delivery or cancel an unfinished report. This addresses the first push being cut short, separately from #1291's already-committed-content retry fix.

Signed-off-by: ydflow <314143294+ydflow@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

No findings in the scoped PR diff.

The PR description provides sufficient testing evidence: built-CLI subprocess runs cover slow report success and rejection, acknowledgement, lock cleanup, and retry. Additional provider/agent coverage is explicitly deferred to CI.

Review was read-only; I did not run, build, or install anything from the PR.

@SaulMoro

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

@ydflow Thanks. This and #1323 fix the same issue (#1322), and both edit the void usageReport.then(...) line in src/pull.ts, so only one can land as is. We approved #1323 because it is the smaller change: it waits in the pull handler and leaves index.ts, the docs and the fixtures alone.

We ran this head too. The real-CLI repro passes, report-timeout.test.ts passes 21/21, and the new cases fail on base. Your rejected-push case is the part #1323 lacks, and it passes on #1323's head. It would be welcome as a follow-up on top of #1323 once that lands, and this PR could then be closed.

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.

[bug] A session-start usage report is killed by the hook-dispatch exit before its push lands

3 participants