fix(git-sync): git-http stream error handlers + close test gaps (#119 review)

Addresses the stability + test-coverage warnings from the #119 review: - git-http-backend.service.ts: add `'error'` handlers to child.stdout/stderr. An EventEmitter 'error' with no listener (e.g. EPIPE when the client aborts mid-response) is rethrown by Node as an uncaught exception and crashes the process; now swallowed + logged (never echoed to the client). - TEST INFRA: a jest setupFile shims `navigator`/`MessageChannel` for the `node` testEnvironment. react-dom@18 reads `navigator` at module-init (pulled in via @docmost/editor-ext -> @tiptap/react), so every spec transitively importing the conversion engine — including git-http.service.spec.ts — previously FAILED TO LOAD ("navigator is not defined") and ran ZERO tests. With the shim those specs now run (git-sync integration: 11 suites / 133 tests green). - git-http.service.spec.ts: cover the 503 lock-held push path — `ingestExternalPush` rejecting `GitSyncLockHeldError` -> 503 + Retry-After + "git-sync busy, retry", no double header write (+ the already-headers-sent no-rewrite path). - git-http-backend.service.spec.ts: unit-test run() — child 'error'/'close' before headers -> 500; normal CGI parse+stream; stdout/stderr 'error' (EPIPE) swallowed; synchronous spawn throw -> 500. - page-change.listener.ts: implement OnModuleDestroy to clearTimeout all pending debounce timers on shutdown (+ test). - .env.example: vaults are non-bare working repos, not "bare repos". (Docs deleted by the stray commit were restored in 9cdbce54.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-24 13:25:04 +03:00
parent 842f014b62
commit 46855652d7
8 changed files with 327 additions and 3 deletions
@@ -1,11 +1,186 @@
 // Unit tests for the pure CGI-response helpers used by GitHttpBackendService.
 // The header/body split MUST treat the body as binary (Buffer) and never
 // stringify it; the Status: header sets the HTTP status (default 200).
+import { EventEmitter } from 'node:events';
+import { spawn } from 'node:child_process';
+
+// Mock the spawn boundary so run() never launches a real `git http-backend`; the
+// fake child lets us drive every stdout/stderr/error/close branch by hand.
+jest.mock('node:child_process', () => ({ spawn: jest.fn() }));
+// vaultGitEnv just builds the CGI env overlay; stub it to a passthrough so the
+// service constructs without the real engine.
+jest.mock('@docmost/git-sync', () => ({
+  vaultGitEnv: (overlay: Record<string, string>) => overlay,
+}));
+
 import {
  parseCgiResponse,
  splitCgiBuffer,
  buildGitBackendCgiEnv,
+  GitHttpBackendService,
 } from './git-http-backend.service';
+import { Logger } from '@nestjs/common';
+import type { GitHttpBackendRequest } from './git-http-backend.service';
+
+const spawnMock = spawn as unknown as jest.Mock;
+
+/** A fake `git http-backend` child: EventEmitter + stdout/stderr/stdin streams. */
+function fakeChild() {
+  const child = new EventEmitter() as any;
+  child.stdout = new EventEmitter();
+  child.stderr = new EventEmitter();
+  // stdin is written/ended/piped to; capture the calls, swallow nothing.
+  child.stdin = Object.assign(new EventEmitter(), {
+    end: jest.fn(),
+    write: jest.fn(),
+  });
+  return child;
+}
+
+/** A fake raw Node ServerResponse capturing status/headers/body/end. */
+function fakeRes() {
+  const res: any = {
+    headersSent: false,
+    writableEnded: false,
+    statusCode: 200,
+    _headers: {} as Record<string, string>,
+    _written: [] as Buffer[],
+    setHeader: jest.fn((name: string, value: string) => {
+      res._headers[name] = value;
+    }),
+    write: jest.fn((chunk: Buffer) => {
+      res._written.push(chunk);
+      return true;
+    }),
+    end: jest.fn((chunk?: Buffer | string) => {
+      if (chunk !== undefined) res._written.push(chunk as Buffer);
+      res.writableEnded = true;
+    }),
+  };
+  return res;
+}
+
+/** A fake raw Node IncomingMessage (GET => no body piped). */
+function fakeReq() {
+  const req = new EventEmitter() as any;
+  req.pipe = jest.fn();
+  return req;
+}
+
+const baseRequest: GitHttpBackendRequest = {
+  spaceId: 'space-1',
+  subpath: 'info/refs',
+  method: 'GET',
+  queryString: 'service=git-upload-pack',
+  contentType: '',
+  remoteUser: 'alice@example.com',
+};
+
+function buildService() {
+  const env = { getGitSyncDataDir: jest.fn(() => '/vaults') };
+  return new GitHttpBackendService(env as any);
+}
+
+describe('GitHttpBackendService.run', () => {
+  beforeEach(() => {
+    spawnMock.mockReset();
+    jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined);
+    jest.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined);
+  });
+  afterEach(() => jest.restoreAllMocks());
+
+  it('(a) responds 500 when the child errors before any headers were written', async () => {
+    const child = fakeChild();
+    spawnMock.mockReturnValue(child);
+    const service = buildService();
+    const res = fakeRes();
+
+    const p = service.run(baseRequest, fakeReq(), res);
+    // Emit a child 'error' before any stdout -> 500, headers not already sent.
+    child.emit('error', new Error('ENOENT spawn git'));
+    await p;
+
+    expect(res.statusCode).toBe(500);
+    expect(res._headers['Content-Type']).toBe('text/plain');
+    expect(res.end).toHaveBeenCalledWith('Internal server error');
+  });
+
+  it('(a) responds 500 when the child closes before a complete CGI header block', async () => {
+    const child = fakeChild();
+    spawnMock.mockReturnValue(child);
+    const service = buildService();
+    const res = fakeRes();
+
+    const p = service.run(baseRequest, fakeReq(), res);
+    // stderr diagnostics, then a close with no valid CGI output -> 500.
+    child.stderr.emit('data', Buffer.from('fatal: boom'));
+    child.emit('close', 128);
+    await p;
+
+    expect(res.statusCode).toBe(500);
+    expect(res.end).toHaveBeenCalledWith('Internal server error');
+  });
+
+  it('(b) parses the CGI header block, sets status/headers, writes the body', async () => {
+    const child = fakeChild();
+    spawnMock.mockReturnValue(child);
+    const service = buildService();
+    const res = fakeRes();
+
+    const p = service.run(baseRequest, fakeReq(), res);
+    // A full CGI response: status line + header + blank line + body.
+    child.stdout.emit(
+      'data',
+      Buffer.from(
+        'Status: 200 OK\r\nContent-Type: application/x-git-upload-pack-advertisement\r\n\r\nPACKBODY',
+        'utf8',
+      ),
+    );
+    child.emit('close', 0);
+    await p;
+
+    expect(res.statusCode).toBe(200);
+    expect(res._headers['Content-Type']).toBe(
+      'application/x-git-upload-pack-advertisement',
+    );
+    expect(Buffer.concat(res._written.map((c) => Buffer.from(c))).toString()).toContain(
+      'PACKBODY',
+    );
+    expect(res.writableEnded).toBe(true);
+  });
+
+  it('(c) swallows a stdout stream error (EPIPE) without throwing or 500ing', async () => {
+    const child = fakeChild();
+    spawnMock.mockReturnValue(child);
+    const service = buildService();
+    const res = fakeRes();
+    const warnSpy = jest.spyOn(Logger.prototype, 'warn');
+
+    const p = service.run(baseRequest, fakeReq(), res);
+    // The stdout 'error' handler must absorb this — no unhandled throw, no 500.
+    expect(() => child.stdout.emit('error', new Error('EPIPE'))).not.toThrow();
+    expect(() => child.stderr.emit('error', new Error('EPIPE'))).not.toThrow();
+    expect(warnSpy).toHaveBeenCalled();
+    expect(res.statusCode).not.toBe(500);
+
+    // Let run() settle so the promise does not dangle.
+    child.emit('close', 0);
+    await p;
+  });
+
+  it('spawn throwing synchronously -> 500 (spawn-failed)', async () => {
+    spawnMock.mockImplementation(() => {
+      throw new Error('spawn EACCES');
+    });
+    const service = buildService();
+    const res = fakeRes();
+
+    await service.run(baseRequest, fakeReq(), res);
+
+    expect(res.statusCode).toBe(500);
+    expect(res.end).toHaveBeenCalledWith('Internal server error');
+  });
+});

 describe('buildGitBackendCgiEnv', () => {
  const base = {
@@ -201,6 +201,13 @@ export class GitHttpBackendService {
          flushHeadersAndBody(chunk);
        }
      });
+      // A stream 'error' (e.g. EPIPE when the client aborts mid-response) is an
+      // EventEmitter 'error' with no listener -> Node rethrows it as an uncaught
+      // exception and crashes the process. Swallow + log it (never echo to the
+      // client); child.on('close')/'error' below drives the actual cleanup.
+      child.stdout?.on('error', (err) => {
+        this.logger.warn(`git http-backend stdout stream error: ${err.message}`);
+      });

      let stderr = '';
      child.stderr?.on('data', (chunk: Buffer) => {
@@ -208,6 +215,9 @@ export class GitHttpBackendService {
        // CGI errors here. We do NOT log the request body or any credentials.
        if (stderr.length < 8192) stderr += chunk.toString('utf8');
      });
+      child.stderr?.on('error', (err) => {
+        this.logger.warn(`git http-backend stderr stream error: ${err.message}`);
+      });

      child.on('error', (err) => {
        if (!headerParsed && !rawRes.headersSent) {
@@ -14,6 +14,7 @@ import {
  SpaceCaslSubject,
 } from '../../../core/casl/interfaces/space-ability.type';
 import { GitHttpService } from './git-http.service';
+import { GitSyncLockHeldError } from '../services/git-sync.orchestrator';

 type AnyMock = jest.Mock;

@@ -353,6 +354,66 @@ describe('GitHttpService.handle', () => {
    expect(workspaceId).toBe('ws-1');
  });

+  it('a push that loses the lock -> 503 with Retry-After and a busy body (headers not written twice)', async () => {
+    const built = build({ abilityCan: true });
+    // The lock could not be acquired: the receive-pack closure never ran, so the
+    // response is still unwritten and the handler must answer 503 itself.
+    built.orchestrator.ingestExternalPush.mockRejectedValue(
+      new GitSyncLockHeldError('space-1'),
+    );
+    const { reply, state } = fakeReply();
+    const req = fakeRequest({
+      url: '/git/space-1.git/git-receive-pack',
+      method: 'POST',
+      authorization: basic('dev@example.com', 'pw'),
+    });
+
+    await built.service.handle(req, reply);
+
+    // It hijacked and went through the orchestrator (write path), but the lock
+    // was held so the backend never ran.
+    expect(state.hijacked).toBe(true);
+    expect(built.orchestrator.ingestExternalPush).toHaveBeenCalledTimes(1);
+    expect(built.backend.run).not.toHaveBeenCalled();
+
+    // 503 + Retry-After were written on the raw response (headersSent was false).
+    const raw = reply.raw as any;
+    expect(raw.statusCode).toBe(503);
+    expect(raw.setHeader).toHaveBeenCalledWith('Content-Type', 'text/plain');
+    expect(raw.setHeader).toHaveBeenCalledWith('Retry-After', '1');
+    // The body carries the busy/retry message and the response was ended once.
+    expect(raw.end).toHaveBeenCalledTimes(1);
+    expect(raw.end).toHaveBeenCalledWith('git-sync busy, retry');
+    // Exactly the two headers above were set — no double write of headers.
+    expect(raw.setHeader).toHaveBeenCalledTimes(2);
+  });
+
+  it('does NOT rewrite the 503 status/headers when the response is already sent', async () => {
+    const built = build({ abilityCan: true });
+    built.orchestrator.ingestExternalPush.mockRejectedValue(
+      new GitSyncLockHeldError('space-1'),
+    );
+    const { reply } = fakeReply();
+    // Simulate the (defensive) case where headers were already flushed: the
+    // handler must skip statusCode/setHeader and only end() the socket.
+    const raw = reply.raw as any;
+    raw.headersSent = true;
+    const req = fakeRequest({
+      url: '/git/space-1.git/git-receive-pack',
+      method: 'POST',
+      authorization: basic('dev@example.com', 'pw'),
+    });
+
+    await built.service.handle(req, reply);
+
+    // No header writes when headersSent is already true (no "headers already
+    // sent" double-write path), but the body/end still runs.
+    expect(raw.setHeader).not.toHaveBeenCalled();
+    expect(raw.statusCode).toBe(200); // untouched default from the fake
+    expect(raw.end).toHaveBeenCalledTimes(1);
+    expect(raw.end).toHaveBeenCalledWith('git-sync busy, retry');
+  });
+
  it('an unresolvable workspace -> 401 (credentials cannot be validated without one)', async () => {
    const built = build({ workspace: null });
    const { reply, state } = fakeReply();