Re: [PR] fix(runtime-host): reap stale Runtime Host control directories [maka]

["Astro-Han (via GitHub)" <gi...@apache.org>]

Astro-Han commented on code in PR #5268:
URL: https://github.com/apache/maka/pull/5268#discussion_r4174892305


##########
packages/storage/src/root-authority.ts:
##########
@@ -816,12 +1027,41 @@ async function acquireStateRootLock<K extends StorageRootKind>(
 
   let compatibilityHandle: FileHandle | undefined;
   let controlDirectory: string;
+  let replacementRetries = 0;
+  let busyRetries = 0;
   try {
-    ({ controlDirectory } = await prepareStorageRootControlDirectory(capability));
-    compatibilityHandle = await tryAcquireStableRootLock(
-      join(controlDirectory, 'owner.lock'),
-      access,
-    );
+    for (;;) {
+      ({ controlDirectory } = await prepareStorageRootControlDirectory(capability));
+      try {
+        compatibilityHandle = await tryAcquireStableRootLock(
+          join(controlDirectory, 'owner.lock'),
+          access,
+        );
+        if (compatibilityHandle) break;
+        // Holding the durable lock rules out a current owner, but a reaper can
+        // briefly hold the compatibility lock while it quarantines stale state.
+        // Bound the wait so an older owner that only knows this lock still wins.
+        if (busyRetries >= ROOT_CONTROL_COMPATIBILITY_LOCK_BUSY_RETRIES) break;

Review Comment:
   P3: this retry window (4 x 25ms) is a timing heuristic, and nothing on the reaper side keeps it within that window. The reaper holds `owner.lock` across about 15 awaited fs operations (`:1225-1282`), in another Host process that runs the sweep right after its own startup (`candidate.ts:91`). Event-loop or threadpool queueing, or a slow home directory, can push that past 100ms. The Candidate then still exits as a loser while no owner exists. That is very unlikely, since the same root must be more than 24h stale and the timing must collide. Still, a matching bound in the reaper would turn this window into a guarantee. For example, record the time when `owner.lock` is acquired, and return `busy` (removing the empty claim) before the rename once more than about 50ms has passed. A debug log when busy retries run out would also help diagnose the residual case.



##########
packages/storage/src/root-authority.ts:
##########
@@ -913,6 +1188,302 @@ async function tryAcquireStableRootLock(
   }
 }
 
+type QuarantineRootControlDirectoryResult =
+  | { kind: 'quarantined'; claimDirectory: string; claimHandle: FileHandle }
+  | { kind: 'busy' }
+  | { kind: 'skipped' };
+
+interface QuarantineRootControlDirectoryInput {
+  controlRoot: string;
+  rootId: string;
+  expectedDirectoryIdentity?: BigIntStats;
+  minimumDirectoryAgeMs?: number;
+  heldOwnerHandle?: FileHandle;
+}
+
+async function quarantineRootControlDirectory(
+  input: QuarantineRootControlDirectoryInput,
+): Promise<QuarantineRootControlDirectoryResult> {
+  if (!ROOT_ID_PATTERN.test(input.rootId)) return { kind: 'skipped' };
+  const controlRoot = resolve(input.controlRoot);
+  const controlDirectory = join(controlRoot, input.rootId);
+  if (parse(controlDirectory).dir !== controlRoot) return { kind: 'skipped' };
+
+  const initialDirectoryStat =
+    input.expectedDirectoryIdentity ?? (await lstatPathIfPresent(controlDirectory));
+  if (!initialDirectoryStat?.isDirectory() || initialDirectoryStat.isSymbolicLink()) {
+    return { kind: 'skipped' };
+  }
+
+  let ownerHandle = input.heldOwnerHandle;
+  const ownsOwnerHandle = ownerHandle === undefined;
+  let writerHandle: FileHandle | undefined;
+  try {
+    if (ownerHandle) {
+      await assertStableLockArtifact(ownerHandle, join(controlDirectory, 'owner.lock'));
+    } else {
+      ownerHandle = await tryAcquireStableRootLock(join(controlDirectory, 'owner.lock'), 'write');
+      if (!ownerHandle) return { kind: 'busy' };
+    }
+
+    writerHandle = await tryAcquireStableRootLock(
+      join(controlDirectory, ARTIFACT_WRITER_LOCK_FILE),
+      'write',
+    );
+    if (!writerHandle) return { kind: 'busy' };
+
+    const currentDirectoryStat = await lstatPathIfPresent(controlDirectory);
+    if (!sameFilesystemIdentity(initialDirectoryStat, currentDirectoryStat)) {
+      return { kind: 'skipped' };
+    }
+    if (!(await containsOnlyDisposableRootControlEntries(controlDirectory))) {
+      return { kind: 'skipped' };
+    }
+    const directoryStatBeforeClaim = await lstatPathIfPresent(controlDirectory);
+    if (
+      directoryStatBeforeClaim === undefined ||
+      !sameFilesystemIdentity(initialDirectoryStat, directoryStatBeforeClaim)
+    ) {
+      return { kind: 'skipped' };
+    }
+    await Promise.all([
+      assertStableLockArtifact(ownerHandle, join(controlDirectory, 'owner.lock')),
+      assertStableLockArtifact(writerHandle, join(controlDirectory, ARTIFACT_WRITER_LOCK_FILE)),
+    ]);
+
+    const claimDirectory = await createRootControlReapClaim(controlRoot);
+    const claimHandle = await tryAcquireStableRootLock(
+      join(claimDirectory, ROOT_CONTROL_CLAIM_LOCK),
+      'write',
+    );
+    if (!claimHandle) return { kind: 'busy' };
+    try {
+      await assertDirectoryIdentity(controlDirectory, initialDirectoryStat);
+      await assertStableLockArtifact(ownerHandle, join(controlDirectory, 'owner.lock'));
+      await assertStableLockArtifact(
+        writerHandle,
+        join(controlDirectory, ARTIFACT_WRITER_LOCK_FILE),
+      );
+      if (!(await containsOnlyDisposableRootControlEntries(controlDirectory))) {
+        await removeEmptyRootControlReapClaim(claimDirectory, claimHandle);
+        return { kind: 'skipped' };
+      }
+      const preRenameDirectoryStat = await lstatPathIfPresent(controlDirectory);
+      if (
+        input.minimumDirectoryAgeMs !== undefined &&
+        (preRenameDirectoryStat === undefined ||
+          !sameFilesystemIdentity(initialDirectoryStat, preRenameDirectoryStat) ||
+          (preRenameDirectoryStat.mtimeNs !== directoryStatBeforeClaim.mtimeNs &&
+            Date.now() - Number(preRenameDirectoryStat.mtimeMs) < input.minimumDirectoryAgeMs))
+      ) {
+        await removeEmptyRootControlReapClaim(claimDirectory, claimHandle);
+        return { kind: 'skipped' };
+      }
+      await rename(controlDirectory, join(claimDirectory, input.rootId));
+    } catch (error) {
+      if (isMissingPathError(error) || isNodeError(error, 'EBUSY') || isNodeError(error, 'EPERM')) {
+        await removeEmptyRootControlReapClaim(claimDirectory, claimHandle);
+        return isMissingPathError(error) ? { kind: 'skipped' } : { kind: 'busy' };
+      }
+      await closeReapLock(claimHandle);

Review Comment:
   P3 (minor): the cleanup now covers rename ENOENT/EBUSY/EPERM. This branch, though, still closes the lock and rethrows without removing the empty `.reap/<uuid>/.claim.lock`. That happens for `invalid_lock_artifact` from `assertDirectoryIdentity` / `assertStableLockArtifact`, and `!claimHandle` at `:1259` has the same effect. These paths realistically need external tampering, and the 24h claim sweep collects them. A best-effort `removeEmptyRootControlReapClaim` here, which must not mask the original error, would make the doc statement at line 140 true for every path.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org