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>

.env.example

@@ -210,7 +210,8 @@ MCP_DOCMOST_PASSWORD=
 # operations (create / move / rename / delete) are attributed to.
 # GIT_SYNC_SERVICE_USER_ID=
 #
-# Where the per-space bare repos / working vaults live.
+# Where the per-space working vaults live (non-bare repos; the engine needs a
+# working tree).
 # Defaults to "<DATA_DIR or ./data>/git-sync".
 # GIT_SYNC_DATA_DIR=
 #

apps/server/package.json

@@ -199,6 +199,9 @@
     ],
     "coverageDirectory": "../coverage",
     "testEnvironment": "node",
+    "setupFiles": [
+      "<rootDir>/../test/jest.setup.ts"
+    ],
     "moduleNameMapper": {
       "^@docmost/db/(.*)$": "<rootDir>/database/$1",
       "^@docmost/transactional/(.*)$": "<rootDir>/integrations/transactional/$1",

apps/server/src/integrations/git-sync/http/git-http-backend.service.spec.ts

@@ -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 = {

apps/server/src/integrations/git-sync/http/git-http-backend.service.ts

@@ -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) {

apps/server/src/integrations/git-sync/http/git-http.service.spec.ts

@@ -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();

apps/server/src/integrations/git-sync/listeners/page-change.listener.spec.ts

@@ -196,6 +196,38 @@ describe('PageChangeListener', () => {
     });
   });
 
+  describe('onModuleDestroy', () => {
+    it('clears every pending debounce timer and empties the map', async () => {
+      jest.useFakeTimers();
+      const clearSpy = jest.spyOn(global, 'clearTimeout');
+      try {
+        const { listener, orchestrator, pageRepo } = build({ debounceMs: 500 });
+        pageRepo.findById.mockResolvedValue({
+          id: 'p1',
+          spaceId: 'space-1',
+          workspaceId: 'ws-1',
+          lastUpdatedSource: 'user',
+        });
+
+        // Schedule a pending cycle, then tear the module down before it fires.
+        await listener.handlePageEvent({ pageId: 'p1', workspaceId: 'ws-1' });
+        clearSpy.mockClear(); // ignore any clears done by schedule() itself
+
+        listener.onModuleDestroy();
+
+        // The pending timer was cleared and the map drained, so advancing past
+        // the debounce window fires NO cycle.
+        expect(clearSpy).toHaveBeenCalledTimes(1);
+        expect((listener as any).debounce.size).toBe(0);
+        jest.advanceTimersByTime(500);
+        expect(orchestrator.runOnce).not.toHaveBeenCalled();
+      } finally {
+        clearSpy.mockRestore();
+        jest.useRealTimers();
+      }
+    });
+  });
+
   describe('error swallowing', () => {
     it('does not throw and logs a warning when findById throws', async () => {
       const warnSpy = jest

apps/server/src/integrations/git-sync/listeners/page-change.listener.ts

@@ -1,4 +1,4 @@
-import { Injectable, Logger } from '@nestjs/common';
+import { Injectable, Logger, OnModuleDestroy } from '@nestjs/common';
 import { OnEvent } from '@nestjs/event-emitter';
 import { PageRepo } from '@docmost/db/repos/page/page.repo';
 import { EnvironmentService } from '../../environment/environment.service';
@@ -36,7 +36,7 @@ interface PageEventLike {
  * here. The poll-safety interval still converges anything this guard drops.
  */
 @Injectable()
-export class PageChangeListener {
+export class PageChangeListener implements OnModuleDestroy {
   private readonly logger = new Logger(PageChangeListener.name);
   // spaceId -> pending debounce timer. The cycle closes over its own
   // workspaceId, so the timer handle is all the map needs to track.
@@ -113,6 +113,19 @@ export class PageChangeListener {
     return undefined;
   }
 
+  /**
+   * On shutdown, clear every pending debounce timer so a not-yet-fired cycle does
+   * not run against a tearing-down module. The timers are already `.unref()`'d (so
+   * they never block process exit), but clearing them also drops the dangling
+   * references and prevents a late `runOnce` from firing post-destroy.
+   */
+  onModuleDestroy(): void {
+    for (const timer of this.debounce.values()) {
+      clearTimeout(timer);
+    }
+    this.debounce.clear();
+  }
+
   /**
    * Debounce per space: a new event resets the timer so a burst collapses into a
    * single cycle. On fire, `runOnce` is enqueued (it internally serializes via the

apps/server/test/jest.setup.ts

@@ -0,0 +1,29 @@
+// Jest global setup (runs before each test module loads).
+//
+// react-dom@18 (pulled in transitively via @docmost/editor-ext -> @tiptap/react
+// -> react-dom, e.g. through the math node) reads `navigator` at MODULE-INIT
+// time. The server jest config uses `testEnvironment: "node"`, which has no
+// `navigator`, so ANY spec that transitively imports the editor schema/engine
+// (e.g. the git-sync HTTP service specs, which reach the conversion engine)
+// fails to LOAD with "ReferenceError: navigator is not defined". These specs
+// never exercise the DOM — they just can't survive the import. Provide the
+// minimal browser globals those modules touch at import so the specs run.
+/* eslint-disable @typescript-eslint/no-explicit-any */
+const g = globalThis as any;
+
+if (typeof g.navigator === "undefined") {
+  // react-dom only reads navigator.userAgent at init; keep it minimal.
+  Object.defineProperty(g, "navigator", {
+    value: { userAgent: "node", platform: "node" },
+    configurable: true,
+    writable: true,
+  });
+}
+
+if (typeof g.MessageChannel === "undefined") {
+  // react-dom's scheduler references MessageChannel at init in some builds.
+  g.MessageChannel = class {
+    port1 = { postMessage() {}, close() {}, onmessage: null };
+    port2 = { postMessage() {}, close() {}, onmessage: null };
+  };
+}