fix(git-sync): kill spurious marker-leaking conflict, concurrent-edit loss, flapping HEAD
Three more git-sync QA defects from the 2nd live pass on PR #119, plus a callout-fidelity nit: 1. SPURIOUS conflict leaked raw markers into canonical main (root cause). On an ordinary round-trip the only difference between the docmost mirror (normalize- on-write) and a user's raw push is trailing/empty-line normalization, which made git's line-based docmost->main merge CONFLICT, and the wedge fix then committed the file WITH literal <<<<<<< / ======= / >>>>>>> markers onto main (git and the DB silently diverged for cycles). Fix: on a conflict, normalize trailing/empty lines on BOTH sides (showStage :2:/:3:) before comparing — a trailing-only diff is recognized as spurious and resolved to the clean normalized form. A GENUINE same-block conflict is auto-resolved to OURS (git wins, mirroring the live-doc 3-way rule); the docmost side stays on the `docmost` branch + page history. Raw markers NEVER reach main again. 2. Concurrent UI<->git edit silently lost the UI side. The git->Docmost 3-way merge ran against a live Y.Doc that hadn't yet received the user's debounced in-flight edit, so git clean-applied (no conflict detected) and the edit vanished even on a different block. Fix: flush the pending debounced store before the merge so the in-flight edit is drained into the live doc first — a different-block edit is merged, a same-block one is detected and pinned to history (recoverable). 3. Smart-HTTP HEAD flapped to the read-only `docmost` mirror (~1/4 of clones). The engine transiently checks out `docmost` mid-pull and the host advertises whatever HEAD resolves to. Fix: VaultGit.pinHeadToMain(); the cycle restores HEAD->main in a finally; and the upload-pack ref advertisement is served HEAD-pinned under the per-space lock so it can never observe a mid-cycle HEAD. 4. (callout) clampCalloutType now mirrors the editor's GITHUB_ALERT_TYPE_MAP for non-schema aliases (tip->success, caution->danger, important->info) instead of flatly collapsing to info. The editor schema genuinely supports only the six banner types, so unknown types still fall back to info (by design). Tests: deterministic real-git trailing-blank round-trip (no conflict, no markers, in sync over 2 cycles) + genuine-conflict no-marker-leak; HEAD advertisement stability; pre/post-flush concurrent-edit survival; serveReadAdvertisement lock pin; widened callout-alias coverage. Engine vitest + server tsc + collaboration / git-http / orchestrator specs all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -118,6 +118,7 @@ function build(opts: BuildOptions = {}): Built {
|
||||
ensureBranch: jest.fn(async () => undefined),
|
||||
checkout: jest.fn(async () => undefined),
|
||||
listTrackedFiles: jest.fn(async () => []),
|
||||
pinHeadToMain: jest.fn(async () => undefined),
|
||||
...(vaultOverrides as Record<string, AnyMock>),
|
||||
};
|
||||
const vaultRegistry = {
|
||||
@@ -380,6 +381,11 @@ describe('GitSyncOrchestrator', () => {
|
||||
expect(order).toEqual(['receive-pack', 'cycle']);
|
||||
});
|
||||
|
||||
// Explicit timeout: ingestExternalPush exhausts the full bounded
|
||||
// acquire-retry budget (GIT_SYNC_PUSH_LOCK_RETRY_TOTAL_MS = 5_000ms) before it
|
||||
// gives up and throws, which races jest's DEFAULT 5_000ms test timeout — flaky
|
||||
// on a loaded/slow runner. Give it headroom so it deterministically observes
|
||||
// the eventual LockHeldError instead of timing out first.
|
||||
it('throws GitSyncLockHeldError and does NOT run the receive-pack when the lock is held', async () => {
|
||||
const built = build();
|
||||
built.redis.set.mockResolvedValue(null); // acquire fails → lock-held
|
||||
@@ -392,7 +398,7 @@ describe('GitSyncOrchestrator', () => {
|
||||
// We must never write to the working tree concurrently with a cycle.
|
||||
expect(runReceivePack).not.toHaveBeenCalled();
|
||||
expect(runCycleMock).not.toHaveBeenCalled();
|
||||
});
|
||||
}, 15_000);
|
||||
|
||||
it('swallows a post-push cycle error (the push is durable; poll retries)', async () => {
|
||||
jest.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined);
|
||||
@@ -444,6 +450,37 @@ describe('GitSyncOrchestrator', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('serveReadAdvertisement (bug #3 — stable advertised HEAD)', () => {
|
||||
it('pins HEAD to main and serves under the space lock', async () => {
|
||||
const built = build();
|
||||
const serve = jest.fn(async () => undefined);
|
||||
|
||||
await built.orchestrator.serveReadAdvertisement('space-1', serve);
|
||||
|
||||
// The lock was taken (redis SET NX) and released (CAS eval).
|
||||
expect(built.redis.set).toHaveBeenCalledTimes(1);
|
||||
expect(built.redis.eval).toHaveBeenCalled();
|
||||
// HEAD pinned BEFORE serving, on the right vault.
|
||||
expect(built.vaultRegistry.getVault).toHaveBeenCalledWith('space-1');
|
||||
expect(built.vault.pinHeadToMain).toHaveBeenCalledTimes(1);
|
||||
expect(serve).toHaveBeenCalledTimes(1);
|
||||
const pinOrder = built.vault.pinHeadToMain.mock.invocationCallOrder[0];
|
||||
const serveOrder = serve.mock.invocationCallOrder[0];
|
||||
expect(pinOrder).toBeLessThan(serveOrder);
|
||||
});
|
||||
|
||||
it('serves WITHOUT a pin/lock when git-sync is globally disabled', async () => {
|
||||
const built = build({ enabled: false });
|
||||
const serve = jest.fn(async () => undefined);
|
||||
|
||||
await built.orchestrator.serveReadAdvertisement('space-1', serve);
|
||||
|
||||
expect(serve).toHaveBeenCalledTimes(1);
|
||||
expect(built.redis.set).not.toHaveBeenCalled();
|
||||
expect(built.vault.pinHeadToMain).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('module lifecycle', () => {
|
||||
it('registers exactly one interval on init and tears it down idempotently on destroy', () => {
|
||||
const built = build();
|
||||
|
||||
Reference in New Issue
Block a user