diff --git a/.env.example b/.env.example index 9bef41b6..e2223a66 100644 --- a/.env.example +++ b/.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 "/git-sync". # GIT_SYNC_DATA_DIR= # diff --git a/apps/server/package.json b/apps/server/package.json index 50b26369..56ad9c87 100644 --- a/apps/server/package.json +++ b/apps/server/package.json @@ -199,6 +199,9 @@ ], "coverageDirectory": "../coverage", "testEnvironment": "node", + "setupFiles": [ + "/../test/jest.setup.ts" + ], "moduleNameMapper": { "^@docmost/db/(.*)$": "/database/$1", "^@docmost/transactional/(.*)$": "/integrations/transactional/$1", diff --git a/apps/server/src/integrations/git-sync/http/git-http-backend.service.spec.ts b/apps/server/src/integrations/git-sync/http/git-http-backend.service.spec.ts index 7b90e1ae..03f4dc25 100644 --- a/apps/server/src/integrations/git-sync/http/git-http-backend.service.spec.ts +++ b/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) => 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, + _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 = { diff --git a/apps/server/src/integrations/git-sync/http/git-http-backend.service.ts b/apps/server/src/integrations/git-sync/http/git-http-backend.service.ts index 5d13ab74..80bc40da 100644 --- a/apps/server/src/integrations/git-sync/http/git-http-backend.service.ts +++ b/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) { diff --git a/apps/server/src/integrations/git-sync/http/git-http.service.spec.ts b/apps/server/src/integrations/git-sync/http/git-http.service.spec.ts index 27ae597f..538fd040 100644 --- a/apps/server/src/integrations/git-sync/http/git-http.service.spec.ts +++ b/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(); diff --git a/apps/server/src/integrations/git-sync/listeners/page-change.listener.spec.ts b/apps/server/src/integrations/git-sync/listeners/page-change.listener.spec.ts index 3f970db8..5fadc74b 100644 --- a/apps/server/src/integrations/git-sync/listeners/page-change.listener.spec.ts +++ b/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 diff --git a/apps/server/src/integrations/git-sync/listeners/page-change.listener.ts b/apps/server/src/integrations/git-sync/listeners/page-change.listener.ts index 027f71de..cd18fff7 100644 --- a/apps/server/src/integrations/git-sync/listeners/page-change.listener.ts +++ b/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 diff --git a/apps/server/test/jest.setup.ts b/apps/server/test/jest.setup.ts new file mode 100644 index 00000000..dfff80e4 --- /dev/null +++ b/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 }; + }; +}