mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
fix(git-watch): close the silent-staleness paths in primary metadata
Two independent reviews converged on the same root cause: nothing bounded how long primary metadata could stay wrong once the shallow watcher stopped reporting. Four distinct paths led there. - A terminal watch error arriving while the status-ref poll was still starting left the repo with status-ref coverage only. handleWatcherError ran its teardown against nulls, then the in-flight poll installed itself, and the fallback guard mistook it for coverage and discarded the fallback. Primary metadata was then never observed again. The guard no longer treats status-ref polling as primary coverage, and startup re-checks watcher liveness after its awaits. - Nothing re-read the six primary files while the watcher was nominally live. A lossy notification path, a dropped batch, or inotify queue overflow raises no error, so the error-driven fallback never fired and the inode rebind sweep does not detect loss. A 15-tick backstop re-stats them, turning permanent staleness into one tick. Measured cost is ~0.2 stats/s/repo against the 3/s the old poll cost. - Reconciliation treated any late-observed entry create as a root replacement, so an ordinary tore down a healthy stream ~30s later and opened a deaf window. It now also requires the root itself to be recreated. - The shallow watcher recorded directory identity from a stat issued after binding, so a replacement in that gap pinned the dead inode's watcher to the new identity and the sweep would never rebind. Identity is now read first, which errs toward a harmless extra rebind. Test helper: replacing the worktrees root frees several inodes at once, so reserving one still let the recreated root reuse its own. It now verifies the inode actually changed. Confirmed on ext4, where three holds were needed.
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
import { watch, type FSWatcher } from 'node:fs'
|
||||
import { statSync, watch, type FSWatcher } from 'node:fs'
|
||||
import { stat } from 'node:fs/promises'
|
||||
import { join } from 'node:path'
|
||||
import type { Event as ParcelWatcherEvent } from '@parcel/watcher'
|
||||
@@ -110,6 +110,15 @@ export function startShallowWatcher(
|
||||
}
|
||||
}
|
||||
|
||||
const directoryIdentitySync = (parent: string): string | null => {
|
||||
try {
|
||||
const entry = statSync(join(rootPath, parent))
|
||||
return entry.isDirectory() ? `${entry.dev}:${entry.ino}` : null
|
||||
} catch {
|
||||
return null
|
||||
}
|
||||
}
|
||||
|
||||
const directoryIdentity = async (parent: string): Promise<string | null> => {
|
||||
try {
|
||||
const entry = await stat(join(rootPath, parent))
|
||||
@@ -148,13 +157,14 @@ export function startShallowWatcher(
|
||||
rebindTimer.unref?.()
|
||||
|
||||
for (const [parent, fileNames] of pathsByDirectory) {
|
||||
// Why: read identity BEFORE binding. If the directory is replaced in the gap,
|
||||
// the recorded identity is stale and the first sweep rebinds — the harmless
|
||||
// direction. Reading after would pin the dead inode's watcher to the new
|
||||
// identity, and the sweep would then never rebind it.
|
||||
const identityBeforeBind = directoryIdentitySync(parent)
|
||||
watchDirectory(parent, fileNames)
|
||||
if (watchers.has(parent)) {
|
||||
void directoryIdentity(parent).then((identity) => {
|
||||
if (!disposed && identity !== null) {
|
||||
boundIdentities.set(parent, identity)
|
||||
}
|
||||
})
|
||||
if (watchers.has(parent) && identityBeforeBind !== null) {
|
||||
boundIdentities.set(parent, identityBeforeBind)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -15,6 +15,14 @@ const PRIMARY_WATCH_OPTIONS = {
|
||||
include: PRIMARY_CHECKOUT_METADATA_FILES
|
||||
}
|
||||
|
||||
// Why: the shallow watcher can stop reporting without ever erroring — a lossy
|
||||
// notification path (network mount, inotify queue overflow in the shared child),
|
||||
// a dropped event batch, or a binding that went deaf between rebind sweeps. None
|
||||
// of those raise, so the error-driven fallback never fires. A bounded re-stat is
|
||||
// the only thing that turns "silently stale forever" into "stale for one tick".
|
||||
// At 15 ticks this is ~0.2 stats/s/repo against the 3/s the old poll cost.
|
||||
const PRIMARY_BACKSTOP_TICKS = 15
|
||||
|
||||
function primaryMetadataEvents(commonDirPath: string): WorktreeBasePollEvent[] {
|
||||
return PRIMARY_CHECKOUT_METADATA_FILES.map((name) => ({
|
||||
type: 'update',
|
||||
@@ -34,6 +42,7 @@ export async function startGitCommonPrimaryWatch(
|
||||
let disposed = false
|
||||
let watcher: WatcherProcessSubscription | null = null
|
||||
let statusRefPolling: WorktreeBaseSubscription | null = null
|
||||
let backstopPolling: WorktreeBaseSubscription | null = null
|
||||
let fallback: WorktreeBaseSubscription | null = null
|
||||
const fallbackFlight = createSingleFlight()
|
||||
|
||||
@@ -47,20 +56,26 @@ export async function startGitCommonPrimaryWatch(
|
||||
visibility,
|
||||
onFullScan
|
||||
).then(async (nextFallback) => {
|
||||
if (disposed || watcher || statusRefPolling) {
|
||||
// `statusRefPolling` deliberately excluded: it covers only status refs,
|
||||
// so its presence is not evidence that primary metadata is covered.
|
||||
if (disposed || watcher) {
|
||||
await nextFallback.unsubscribe()
|
||||
return
|
||||
}
|
||||
void stopWatcherSidePolling()
|
||||
fallback = nextFallback
|
||||
})
|
||||
)
|
||||
|
||||
const stopStatusRefPolling = async (): Promise<void> => {
|
||||
const stopWatcherSidePolling = async (): Promise<void> => {
|
||||
const current = statusRefPolling
|
||||
const currentBackstop = backstopPolling
|
||||
statusRefPolling = null
|
||||
if (current) {
|
||||
await current.unsubscribe().catch(() => {})
|
||||
}
|
||||
backstopPolling = null
|
||||
await Promise.all([
|
||||
current?.unsubscribe().catch(() => {}),
|
||||
currentBackstop?.unsubscribe().catch(() => {})
|
||||
])
|
||||
}
|
||||
|
||||
const handleWatcherError = (error: Error): void => {
|
||||
@@ -76,7 +91,7 @@ export async function startGitCommonPrimaryWatch(
|
||||
if (current) {
|
||||
void current.unsubscribe().catch(() => {})
|
||||
}
|
||||
void stopStatusRefPolling()
|
||||
void stopWatcherSidePolling()
|
||||
.then(() => startFallback())
|
||||
.catch(() => {})
|
||||
}
|
||||
@@ -108,15 +123,41 @@ export async function startGitCommonPrimaryWatch(
|
||||
}
|
||||
)
|
||||
if (watcher && !disposed && !fallbackFlight.pending()) {
|
||||
statusRefPolling = await startGitCommonPrimaryPolling(
|
||||
commonDirPath,
|
||||
getStatusRefPaths,
|
||||
onEvents,
|
||||
pollIntervalMs,
|
||||
visibility,
|
||||
undefined,
|
||||
false
|
||||
)
|
||||
const [nextStatusRefPolling, nextBackstop] = await Promise.all([
|
||||
startGitCommonPrimaryPolling(
|
||||
commonDirPath,
|
||||
getStatusRefPaths,
|
||||
onEvents,
|
||||
pollIntervalMs,
|
||||
visibility,
|
||||
undefined,
|
||||
false
|
||||
),
|
||||
startGitCommonPrimaryPolling(
|
||||
commonDirPath,
|
||||
() => [],
|
||||
onEvents,
|
||||
pollIntervalMs * PRIMARY_BACKSTOP_TICKS,
|
||||
visibility,
|
||||
undefined,
|
||||
true
|
||||
)
|
||||
])
|
||||
// Why: a terminal watch error can land while the two polls above are still
|
||||
// starting. handleWatcherError already ran its teardown against nulls, so
|
||||
// adopting these now would strand the repo with status-ref coverage only.
|
||||
if (disposed || !watcher) {
|
||||
await Promise.all([
|
||||
nextStatusRefPolling.unsubscribe().catch(() => {}),
|
||||
nextBackstop.unsubscribe().catch(() => {})
|
||||
])
|
||||
if (!disposed) {
|
||||
await startFallback()
|
||||
}
|
||||
} else {
|
||||
statusRefPolling = nextStatusRefPolling
|
||||
backstopPolling = nextBackstop
|
||||
}
|
||||
}
|
||||
} catch (error) {
|
||||
handleWatcherError(error instanceof Error ? error : new Error(String(error)))
|
||||
@@ -130,7 +171,7 @@ export async function startGitCommonPrimaryWatch(
|
||||
if (current) {
|
||||
await current.unsubscribe().catch(() => {})
|
||||
}
|
||||
await stopStatusRefPolling()
|
||||
await stopWatcherSidePolling()
|
||||
await fallbackFlight.pending()?.catch(() => {})
|
||||
if (fallback) {
|
||||
await fallback.unsubscribe().catch(() => {})
|
||||
|
||||
@@ -60,12 +60,22 @@ export function createGitCommonWatchReconciliation({
|
||||
const rootWasReplaced =
|
||||
events.some((event) => event.type === 'delete' && event.path === worktreesDir) &&
|
||||
events.some((event) => event.type === 'create' && event.path === worktreesDir)
|
||||
const coarseRootReplacement = events.some(
|
||||
(event) =>
|
||||
event.type === 'create' &&
|
||||
event.path !== worktreesDir &&
|
||||
dirname(event.path) === worktreesDir
|
||||
// Why: this backstop lags the native stream by up to 15 ticks, so it
|
||||
// routinely reports entry creates the stream already delivered. Only
|
||||
// treat them as a replacement when the root itself was also recreated
|
||||
// — otherwise every ordinary `git worktree add` would tear down a
|
||||
// healthy stream and open a deaf window while it resubscribes.
|
||||
const rootRecreated = events.some(
|
||||
(event) => event.type === 'create' && event.path === worktreesDir
|
||||
)
|
||||
const coarseRootReplacement =
|
||||
rootRecreated &&
|
||||
events.some(
|
||||
(event) =>
|
||||
event.type === 'create' &&
|
||||
event.path !== worktreesDir &&
|
||||
dirname(event.path) === worktreesDir
|
||||
)
|
||||
if (rootWasReplaced || coarseRootReplacement) {
|
||||
onRootReplacement()
|
||||
}
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import { appendFile, mkdtemp, mkdir, realpath, rm, writeFile } from 'node:fs/promises'
|
||||
import { appendFile, mkdtemp, mkdir, realpath, rm, stat, writeFile } from 'node:fs/promises'
|
||||
import type * as NodeFsPromises from 'node:fs/promises'
|
||||
import { performance as nodePerformance } from 'node:perf_hooks'
|
||||
import { tmpdir } from 'node:os'
|
||||
@@ -52,15 +52,22 @@ async function replaceWorktreesRoot(
|
||||
worktreesDir: string,
|
||||
retainedEntry: string
|
||||
): Promise<void> {
|
||||
const previousInode = (await stat(worktreesDir)).ino
|
||||
await rm(worktreesDir, { recursive: true })
|
||||
// Linux reuses just-deleted directory inodes. Park each freed inode in a
|
||||
// sibling that is HELD for the rest of the test — releasing it would return it
|
||||
// to the free list, so a later replacement could land back on it and look
|
||||
// unchanged to reconciliation. The dirs are siblings of `worktrees`, which no
|
||||
// poll or watch path enumerates.
|
||||
const inodeReservation = join(commonDir, `worktrees-replacement-inode-${++inodeReservationCount}`)
|
||||
await mkdir(inodeReservation)
|
||||
await mkdir(retainedEntry, { recursive: true })
|
||||
// Linux reuses freed directory inodes, and removing the tree frees several at
|
||||
// once, so the recreated root can land back on its own old inode and look
|
||||
// unchanged to reconciliation. Park inodes in HELD siblings until the root
|
||||
// genuinely differs — verifying beats guessing how many to reserve. The
|
||||
// siblings live next to `worktrees`, which no poll or watch path enumerates.
|
||||
for (let attempt = 0; attempt < 16; attempt++) {
|
||||
await mkdir(retainedEntry, { recursive: true })
|
||||
if ((await stat(worktreesDir)).ino !== previousInode) {
|
||||
return
|
||||
}
|
||||
await rm(worktreesDir, { recursive: true })
|
||||
await mkdir(join(commonDir, `worktrees-inode-hold-${++inodeReservationCount}`))
|
||||
}
|
||||
throw new Error('could not obtain a fresh inode for the replaced worktrees root')
|
||||
}
|
||||
|
||||
function createVisibilityHarness(): {
|
||||
@@ -233,7 +240,7 @@ describe('worktree git-common narrow watch (local native platforms)', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('leaves primary metadata stats to the shallow child on the healthy path', async () => {
|
||||
it('keeps primary metadata off the per-tick path but re-stats it on the backstop', async () => {
|
||||
installSubscribeMock()
|
||||
const commonDir = await makeCommonDir(true)
|
||||
const headPath = join(commonDir, 'HEAD')
|
||||
@@ -242,14 +249,37 @@ describe('worktree git-common narrow watch (local native platforms)', () => {
|
||||
writeFile(headPath, 'ref: refs/heads/main'),
|
||||
writeFile(configPath, '[core]\n')
|
||||
])
|
||||
statCalls.length = 0
|
||||
|
||||
const received: WorktreeBasePollEvent[][] = []
|
||||
await startWatch(commonDir, received)
|
||||
|
||||
expect(statCalls).not.toContain(headPath)
|
||||
expect(statCalls).not.toContain(configPath)
|
||||
expect(primarySubscription().dir).toBe(commonDir)
|
||||
|
||||
// The shallow child owns the fast path: ordinary ticks must not re-stat these.
|
||||
statCalls.length = 0
|
||||
await vi
|
||||
.waitFor(
|
||||
() => {
|
||||
expect(statCalls.length).toBeGreaterThan(0)
|
||||
},
|
||||
{ timeout: 2_000 }
|
||||
)
|
||||
.catch(() => {})
|
||||
const duringOrdinaryTicks = statCalls.filter(
|
||||
(path) => path === headPath || path === configPath
|
||||
).length
|
||||
|
||||
// ...but a bounded backstop must still re-stat them, or a watcher that goes
|
||||
// silently lossy would leave primary metadata stale forever.
|
||||
statCalls.length = 0
|
||||
await vi.waitFor(
|
||||
() => {
|
||||
expect(statCalls).toContain(headPath)
|
||||
expect(statCalls).toContain(configPath)
|
||||
},
|
||||
{ timeout: POLL_MS * RECONCILIATION_TICKS * 6 }
|
||||
)
|
||||
const backstopSamples = statCalls.filter((path) => path === headPath).length
|
||||
expect(duringOrdinaryTicks).toBeLessThan(backstopSamples * RECONCILIATION_TICKS)
|
||||
})
|
||||
|
||||
it('polls only the accepted upstream ref and rebinds without synthetic events', async () => {
|
||||
@@ -376,9 +406,12 @@ describe('worktree git-common narrow watch (local native platforms)', () => {
|
||||
),
|
||||
[]
|
||||
)
|
||||
// Narrow fallback + selected-ref polling + primary backstop, with the narrow
|
||||
// existence poll retired. Waiting on the exact count is what guarantees the
|
||||
// fallback has baselined before the entry below is created.
|
||||
await vi.waitFor(() => {
|
||||
expect(narrowSubscription().unsubscribe).toHaveBeenCalledOnce()
|
||||
expect(visibility.listenerCount()).toBe(3)
|
||||
expect(visibility.listenerCount()).toBe(4)
|
||||
})
|
||||
|
||||
const entryPath = join(worktreesDir, 'fallback-entry')
|
||||
@@ -547,8 +580,9 @@ describe('worktree git-common narrow watch (local native platforms)', () => {
|
||||
visibility.source
|
||||
)
|
||||
|
||||
// Narrow existence + selected-ref polling each park on window visibility.
|
||||
expect(visibility.listenerCount()).toBe(2)
|
||||
// Narrow existence, selected-ref polling, and the primary backstop each park
|
||||
// on window visibility.
|
||||
expect(visibility.listenerCount()).toBe(3)
|
||||
await watch.unsubscribe()
|
||||
expect(visibility.listenerCount()).toBe(0)
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user