diff --git a/CHANGELOG.md b/CHANGELOG.md index 89851c326..8afd2408d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Idle Git reader retirement now completes when the child process closes + before stdin reports its final flush. Storage shutdown no longer waits + indefinitely for that missing stream event. Requires Plumbing 3.3.1. - Runtime-backed Reading values now retain their exact named fields instead of declaring arbitrary dictionary keys to satisfy the recursive snapshot value algebra. Consumer type checks reject undeclared `EntityAdmission` fields, diff --git a/package-lock.json b/package-lock.json index c9197b784..c24db5da6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -17,7 +17,7 @@ "@flyingrobots/bijou-tui": "^7.2.0", "@git-stunts/alfred": "^0.10.4", "@git-stunts/git-cas": "^6.5.10", - "@git-stunts/plumbing": "^3.3.0", + "@git-stunts/plumbing": "^3.3.1", "@git-stunts/trailer-codec": "^2.1.1", "@noble/hashes": "^2.2.0", "boxen": "^7.1.1", @@ -42,7 +42,7 @@ "@types/node": "^22.15.29", "@typescript-eslint/eslint-plugin": "^8.54.0", "@typescript-eslint/parser": "^8.54.0", - "@vitest/coverage-v8": "^4.1.2", + "@vitest/coverage-v8": "^4.1.11", "eslint": "^9.17.0", "eslint-plugin-jsdoc": "^62.8.1", "fast-check": "^4.5.3", @@ -53,7 +53,7 @@ "prettier": "^3.4.2", "typescript": "^5.9.3", "typescript-eslint": "^8.54.0", - "vitest": "^4.0.18", + "vitest": "^4.1.11", "yaml": "^2.9.0" }, "engines": { @@ -580,9 +580,9 @@ } }, "node_modules/@git-stunts/plumbing": { - "version": "3.3.0", - "resolved": "https://registry.npmjs.org/@git-stunts/plumbing/-/plumbing-3.3.0.tgz", - "integrity": "sha512-v/AT3hKgmFKSQ3M+n7n9VgC5Ri7C+NDtZS11Bj1JmT0Xv523hNdjCIaRHSXjAaCpiuXuDKFlj+E8PcBI1+FxbA==", + "version": "3.3.1", + "resolved": "https://registry.npmjs.org/@git-stunts/plumbing/-/plumbing-3.3.1.tgz", + "integrity": "sha512-/pUSgdv4wE0czYywnCMJy4n4XoJud9B9gBpFcbfNQz/ZOF9WHvGuM/9T6KELcIiy2mWBNMUS+syRyLfYzL1Vmw==", "license": "Apache-2.0", "dependencies": { "zod": "^3.24.1" diff --git a/package.json b/package.json index 9a0440354..bd74813f9 100644 --- a/package.json +++ b/package.json @@ -170,7 +170,7 @@ "@flyingrobots/bijou-tui": "^7.2.0", "@git-stunts/alfred": "^0.10.4", "@git-stunts/git-cas": "^6.5.10", - "@git-stunts/plumbing": "^3.3.0", + "@git-stunts/plumbing": "^3.3.1", "@git-stunts/trailer-codec": "^2.1.1", "@noble/hashes": "^2.2.0", "boxen": "^7.1.1", @@ -191,7 +191,7 @@ "@types/node": "^22.15.29", "@typescript-eslint/eslint-plugin": "^8.54.0", "@typescript-eslint/parser": "^8.54.0", - "@vitest/coverage-v8": "^4.1.2", + "@vitest/coverage-v8": "^4.1.11", "eslint": "^9.17.0", "eslint-plugin-jsdoc": "^62.8.1", "fast-check": "^4.5.3", @@ -202,7 +202,7 @@ "prettier": "^3.4.2", "typescript": "^5.9.3", "typescript-eslint": "^8.54.0", - "vitest": "^4.0.18", + "vitest": "^4.1.11", "yaml": "^2.9.0" }, "keywords": [ diff --git a/scripts/GitMachineLocalPathGuard.ts b/scripts/GitMachineLocalPathGuard.ts index 140c8db8b..1ffb9ae45 100644 --- a/scripts/GitMachineLocalPathGuard.ts +++ b/scripts/GitMachineLocalPathGuard.ts @@ -91,7 +91,14 @@ export class GitMachineLocalPathGuard { continue; } - const exclusions = ZERO_OBJECT_PATTERN.test(remoteObject) ? remoteTips : [remoteObject]; + // Exclude everything the remote already holds, not just this ref's old + // tip. An object published on another remote ref — typically a blob that + // reached main and arrived here through a merge — is not outgoing. + // Scanning it again would make every branch that merges main permanently + // unpushable, since the history carrying it cannot be rewritten. + const exclusions = ZERO_OBJECT_PATTERN.test(remoteObject) + ? remoteTips + : [remoteObject, ...remoteTips]; const revisionArguments = [localObject, ...exclusions.map((objectId) => `^${objectId}`)]; const inventory = execFileSync( 'git', diff --git a/test/integration/application/ClosedStdinSchedule.ts b/test/integration/application/ClosedStdinSchedule.ts new file mode 100644 index 000000000..a578d9652 --- /dev/null +++ b/test/integration/application/ClosedStdinSchedule.ts @@ -0,0 +1,60 @@ +import childProcess, { type ChildProcess } from 'node:child_process'; +import { syncBuiltinESMExports } from 'node:module'; +import type { Writable } from 'node:stream'; +import { vi } from 'vitest'; + +/** Forces real Git to close its input before Writable reports a final flush. */ +export default class ClosedStdinSchedule { + private readonly completion = Promise.withResolvers(); + private readonly inputs: Writable[] = []; + private restoreSpawn: () => void = () => {}; + + get closed(): Promise { + return this.completion.promise; + } + + get witnesses(): number { + return this.inputs.length; + } + + get closedWithoutFinish(): number { + return this.inputs.filter((input) => input.closed && !input.writableFinished).length; + } + + install(): void { + const spawn = childProcess.spawn; + const replacement = vi.spyOn(childProcess, 'spawn').mockImplementation((command, args, options) => { + const child = spawn(command, args, options); + if (Array.isArray(args) && args.includes('cat-file') && args.includes('--batch-command')) { + this.control(child); + } + return child; + }); + this.restoreSpawn = () => replacement.mockRestore(); + syncBuiltinESMExports(); + } + + release(): void { + // Rescue the deliberately blocked baseline only after the verdict, so a + // failing regression can still release its owned storage during teardown. + for (const input of this.inputs) { + input.emit('finish'); + } + this.restoreSpawn(); + syncBuiltinESMExports(); + } + + private control(child: ChildProcess): void { + const input = child.stdin; + if (input === null) { + throw new Error('The controlled Git process must have piped stdin'); + } + this.inputs.push(input); + const final = input._final.bind(input); + input._final = () => final(() => { + // Send real EOF, but withhold the final callback until Git closes its + // pipe. This models close-without-finish at the Node stream boundary. + }); + child.once('close', (code) => this.completion.resolve(code)); + } +} diff --git a/test/integration/application/GitTimelineHistoryAdapter.retirement.integration.test.ts b/test/integration/application/GitTimelineHistoryAdapter.retirement.integration.test.ts new file mode 100644 index 000000000..b7c9d3121 --- /dev/null +++ b/test/integration/application/GitTimelineHistoryAdapter.retirement.integration.test.ts @@ -0,0 +1,46 @@ +import { setImmediate as nextTurn } from 'node:timers/promises'; +import { describe, expect, it, onTestFinished, vi } from 'vitest'; +import Plumbing from '@git-stunts/plumbing'; +import GitTimelineHistoryAdapter from '../../../src/infrastructure/adapters/GitTimelineHistoryAdapter.ts'; +import { createTestRepo } from '../api/helpers/setup.ts'; +import ClosedStdinSchedule from './ClosedStdinSchedule.ts'; + +const DEFAULT_SESSION_IDLE_MS = 1000; + +// Medium: real Git in an owned repository; only the idle clock is virtual. +// Oracle: after the owned Git process closes, history close must settle even +// when stdin never emits finish. A pending retirement must not strand close. +describe('Git history session retirement', () => { + it('settles storage closure after an idle reader closes stdin without finish', async () => { + const repository = await createTestRepo('idle-reader-close'); + onTestFinished(() => repository.cleanup()); + let history: GitTimelineHistoryAdapter | null = null; + onTestFinished(() => history?.close()); + const content = 'idle reader lifecycle'; + const oid = await repository.plumbing.execute({ + args: ['hash-object', '-w', '--stdin'], input: content, + }); + // Finished hooks run in reverse order and retain each failure independently. + // Release the controlled stream before storage cleanup can await it. + const schedule = new ClosedStdinSchedule(); + onTestFinished(() => { + vi.useRealTimers(); + }); + onTestFinished(() => schedule.release()); + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); + schedule.install(); + const plumbing = await Plumbing.createDefault({ cwd: repository.tempDir }); + history = new GitTimelineHistoryAdapter({ plumbing }); + expect(new TextDecoder().decode(await history.readBlob(oid))).toBe(content); + expect(schedule.witnesses).toBe(1); + vi.advanceTimersByTime(DEFAULT_SESSION_IDLE_MS); + expect(await schedule.closed).toBe(0); + expect(schedule.closedWithoutFinish).toBe(1); + const outcome = { settled: false }; + const closing = history.close().then(() => { outcome.settled = true; }); + await nextTurn(); + + expect(outcome.settled, 'history close must settle after idle reader exit').toBe(true); + await closing; + }); +}); diff --git a/test/runtime/deno/deno.json b/test/runtime/deno/deno.json index 867edc749..fcecdd5d0 100644 --- a/test/runtime/deno/deno.json +++ b/test/runtime/deno/deno.json @@ -2,7 +2,7 @@ "imports": { "@git-stunts/alfred": "npm:@git-stunts/alfred@^0.10.3", "@git-stunts/git-cas": "npm:@git-stunts/git-cas@^6.5.10", - "@git-stunts/plumbing": "npm:@git-stunts/plumbing@^3.3.0", + "@git-stunts/plumbing": "npm:@git-stunts/plumbing@^3.3.1", "@git-stunts/trailer-codec": "npm:@git-stunts/trailer-codec@^2.1.1", "@noble/hashes/blake3.js": "npm:@noble/hashes@^2.2.0/blake3.js", "cbor-x": "npm:cbor-x@^1.6.0", diff --git a/test/unit/scripts/git-machine-local-path-guard.test.ts b/test/unit/scripts/git-machine-local-path-guard.test.ts index c19287518..240fc4189 100644 --- a/test/unit/scripts/git-machine-local-path-guard.test.ts +++ b/test/unit/scripts/git-machine-local-path-guard.test.ts @@ -103,6 +103,43 @@ describe('Git machine-local path guard', () => { expect(guard.findOutgoingObjects(pushUpdate, 'origin')).toContain(`blob:${leakedBlob}`); }); + it('does not rescan objects the remote already holds on another ref', () => { + // Reproduces the real deadlock: a leaked blob lands on main, a feature + // branch whose published tip predates that merges main, and the blob + // becomes "outgoing" relative to the feature's own remote tip. Rescanning + // it makes every branch that merges main permanently unpushable, because + // the history carrying it cannot be rewritten. + const repository = createRepository(); + writeFileSync(join(repository, 'base.txt'), 'portable base', 'utf8'); + git(repository, 'add', 'base.txt'); + git(repository, 'commit', '--quiet', '-m', 'base'); + const base = gitText(repository, 'rev-parse', 'HEAD'); + // Do not assume the initial branch is named 'main' — init.defaultBranch + // differs between environments, and CI's does not match this machine's. + const trunk = gitText(repository, 'rev-parse', '--abbrev-ref', 'HEAD'); + + // Feature branch published at the base, before the leak exists. + git(repository, 'checkout', '--quiet', '-b', 'feature'); + git(repository, 'update-ref', 'refs/remotes/origin/feature', base); + + // The leak lands on the trunk and is published there. + git(repository, 'checkout', '--quiet', trunk); + writeFileSync(join(repository, 'leak.txt'), personalHome('git', 'legacy'), 'utf8'); + git(repository, 'add', 'leak.txt'); + git(repository, 'commit', '--quiet', '-m', 'historical leak on main'); + git(repository, 'update-ref', 'refs/remotes/origin/main', gitText(repository, 'rev-parse', 'HEAD')); + + // The feature merges the trunk, inheriting the already-published blob. + git(repository, 'checkout', '--quiet', 'feature'); + git(repository, 'merge', '--quiet', '--no-edit', trunk); + const localObject = gitText(repository, 'rev-parse', 'HEAD'); + + const pushUpdate = `refs/heads/feature ${localObject} refs/heads/feature ${base}\n`; + const guard = new GitMachineLocalPathGuard(repository, new MachineLocalPathPolicy()); + + expect(guard.findOutgoingObjects(pushUpdate, 'origin')).toEqual([]); + }); + it('runs the exact outgoing-object scanner from the pre-push hook', () => { const hookPath = fileURLToPath(new URL('../../../scripts/hooks/pre-push', import.meta.url)); const hook = readFileSync(hookPath, 'utf8');