From f9cd3e631899f9728abe419fe9f9be38f4e40895 Mon Sep 17 00:00:00 2001 From: claude-stand Date: Thu, 2 Jul 2026 18:16:59 +0300 Subject: [PATCH] fix(git-sync): forward gzip encoding (#4) + catch read-advertise reject after hijack (#5); fix Export 500 on pages with inline comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Review #4: forward HTTP_CONTENT_ENCODING to git http-backend so it inflates gzip'd RPC bodies — a non-trivial `git pull` no longer fails with `fatal: expected 'packfile'`. (git-http-backend + git-http.service) - Review #5: the read-advertisement branch runs under the space lock AFTER reply.hijack(); a reject there (e.g. Redis down) previously left the socket open forever, hanging every clone/fetch. Mirror the push branch: catch, 500 if unwritten, always end the socket. (git-http.service) - GS-EXPORT-500 (QA): a page with an inline comment mark returned HTTP 500 on Export/copy-as-markdown. The Comment mark's renderHTML took the imperative document.createElement branch server-side (the DOM shim used by generateHTML defines window/document), returning a live node with no content hole that crashed prosemirror-model's DOMSerializer under happy-dom. Gate the imperative branch on a real browser (navigator.userAgent contains 'Mozilla'); the server now uses the static DOMOutputSpec form. Verified: export 200 (was 500). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../git-sync/http/git-http-backend.service.ts | 13 ++++++++ .../git-sync/http/git-http.service.ts | 30 +++++++++++++++++-- .../editor-ext/src/lib/comment/comment.ts | 22 +++++++++++++- 3 files changed, 61 insertions(+), 4 deletions(-) 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 108785ed..cb39686f 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 @@ -86,6 +86,11 @@ export interface GitHttpBackendRequest { contentType: string; /** The Git-Protocol request header value, or undefined when absent. */ gitProtocol?: string; + /** Content-Encoding request header (e.g. `gzip`), or undefined when absent. + * git gzips RPC bodies >1KiB; http-backend only inflates when HTTP_CONTENT_ENCODING + * is present, so it MUST be forwarded or a non-trivial `git pull` fails with + * `fatal: expected 'packfile'` (review #4). */ + contentEncoding?: string; /** Authenticated user email — used as REMOTE_USER (reflog identity). */ remoteUser: string; } @@ -131,6 +136,14 @@ export function buildGitBackendCgiEnv( if (parsed.gitProtocol) { cgiEnv.GIT_PROTOCOL = parsed.gitProtocol; } + // HTTP_CONTENT_ENCODING must be forwarded so git http-backend inflates a + // gzip'd RPC body (git compresses receive-pack/upload-pack bodies >1KiB). + // Without it a non-trivial `git pull` negotiation fails deterministically with + // `fatal: expected 'packfile'` (review #4). The body is piped to stdin as-is + // (no upstream decompression), so the CGI must do the inflate. + if (parsed.contentEncoding) { + cgiEnv.HTTP_CONTENT_ENCODING = parsed.contentEncoding; + } return cgiEnv; } diff --git a/apps/server/src/integrations/git-sync/http/git-http.service.ts b/apps/server/src/integrations/git-sync/http/git-http.service.ts index b1967bfa..a1b76ced 100644 --- a/apps/server/src/integrations/git-sync/http/git-http.service.ts +++ b/apps/server/src/integrations/git-sync/http/git-http.service.ts @@ -345,6 +345,9 @@ export class GitHttpService implements OnModuleDestroy { queryString: this.extractQueryString(req.url), contentType: this.headerValue(req.headers['content-type']) ?? '', gitProtocol: this.headerValue(req.headers['git-protocol']), + // Forward Content-Encoding so http-backend inflates gzip'd RPC bodies + // (review #4) — else a non-trivial `git pull` fails with expected 'packfile'. + contentEncoding: this.headerValue(req.headers['content-encoding']), remoteUser: user.email, }; @@ -389,9 +392,30 @@ export class GitHttpService implements OnModuleDestroy { service === 'git-upload-pack') || parsedPath.subpath === 'HEAD'); if (isReadAdvertise) { - await this.orchestrator.serveReadAdvertisement(spaceId, () => - this.backend.run(backendRequest, rawReq, rawRes), - ); + // The read-advertise path runs under the space lock (to pin HEAD=main). + // AFTER reply.hijack() Fastify no longer manages this response, so a + // rejection here (e.g. SpaceLockService.acquire when Redis is down) would + // otherwise leave the socket open forever — every clone/fetch hangs until + // the client times out (review #5). Mirror the push branch: catch, answer + // 500 if nothing was written yet, and always end the raw socket. + try { + await this.orchestrator.serveReadAdvertisement(spaceId, () => + this.backend.run(backendRequest, rawReq, rawRes), + ); + } catch (err) { + this.logger.error( + `git-sync: read advertisement failed for space ${spaceId}: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + if (!rawRes.headersSent) { + rawRes.statusCode = 500; + rawRes.setHeader('Content-Type', 'text/plain'); + rawRes.end('Internal server error'); + } else if (!rawRes.writableEnded) { + rawRes.end(); + } + } } else { await this.backend.run(backendRequest, rawReq, rawRes); } diff --git a/packages/editor-ext/src/lib/comment/comment.ts b/packages/editor-ext/src/lib/comment/comment.ts index ec896357..f2c1c397 100644 --- a/packages/editor-ext/src/lib/comment/comment.ts +++ b/packages/editor-ext/src/lib/comment/comment.ts @@ -172,7 +172,27 @@ export const Comment = Mark.create({ const commentId = HTMLAttributes?.["data-comment-id"] || null; const resolved = HTMLAttributes?.["data-resolved"] || false; - if (typeof window === "undefined" || typeof document === "undefined") { + // Prefer the static array (DOMOutputSpec) form whenever we are NOT in a real + // interactive browser. Guarding only on `document`/`window` is insufficient + // on the server: `generateHTML()` runs under a DOM shim (happy-dom/jsdom) that + // DEFINES both globals, so the imperative `document.createElement` branch ran + // server-side and its live node + addEventListener crashed the shim's + // DOMSerializer ("Cannot read properties of undefined (reading 'length')"), + // turning every Export / copy-as-markdown of a page with an inline comment + // into an HTTP 500 (QA GS-EXPORT-500). A real browser has a non-empty + // navigator.userAgent; the SSR shims do not — route the server to the safe + // static form while keeping the clickable node in the actual editor. + const isInteractiveBrowser = + typeof window !== "undefined" && + typeof document !== "undefined" && + typeof navigator !== "undefined" && + typeof navigator.userAgent === "string" && + // Real browsers ALL carry "Mozilla" in the UA string (historical); the + // server-side DOM shim used by generateHTML() does not (e.g. "Node.js/22"). + // This keeps the interactive click node ONLY in a true browser and routes + // the server to the crash-free static form. + navigator.userAgent.includes("Mozilla"); + if (!isInteractiveBrowser) { return [ "span", mergeAttributes(this.options.HTMLAttributes, HTMLAttributes, {