fix(env): name unreclaimable layer locks and the re-include cause

docs/env.md

@@ -109,7 +109,17 @@ How it is maintained:
   key is created. Before a `--secret` set or hydrate uses an existing
   checkout-local key that git does not ignore, Hack adds the entry; if it
   cannot, or a later rule re-includes the key, the command is refused and
-  nothing is encrypted with that key
+  nothing is encrypted with that key. The refusal names the cause: either the
+  entry could not be written, or the file already lists the key and a later
+  rule (such as `!.hack.secret.key`) re-includes it, which you need to remove
+- outside git there is nothing to commit, so an existing key does not need the
+  entry and Hack does not recreate a deleted root `.gitignore` for it. Hack
+  does not run when you later turn the directory into a repository with
+  `git init`, so it cannot add the entry at that moment. If the entry is
+  missing then, `hack doctor` reports `env-key-unignored` (see
+  [Key strength and exposure](#secret-key-handling)), and the first `--secret` set or
+  hydrate in the new repository adds the entry before it uses the key, or is
+  refused
 - older repos that added `.hack/.internal/` to the root `.gitignore` keep
   working; the entries are equivalent and nothing is removed
 
@@ -669,9 +679,19 @@ file, change it and replace it while holding that file's write lock,
 The same applies to the legacy `.hack/.env` files. So when several commands change
 different keys of the same file at once, every change is kept. A command
 that finds the lock held waits up to 10 seconds, then stops with a one-line message that
-names the file and says nothing was written; run it again. The lock is released when the
-command ends. If a command is killed while it holds the lock, the next command takes the
-lock over once it confirms that process has exited.
+names the file and its lock and says nothing was written; run it again. The lock is
+released when the command ends. If a command is killed while it holds the lock, the next
+command takes the lock over once it confirms that process has exited.
+
+Hack never removes a lock it cannot prove is abandoned. That includes a lock file that
+is not a complete Hack receipt, one owned by another user or writable by others, and one
+that names a process ID now used by an unrelated live process. In those cases every
+retry would wait and fail the same way, so the message also says what to do: if no
+`hack env` command is running, remove the lock file it names and retry. If a command was
+killed while it was taking over an abandoned lock, it leaves `<lock>.recovery` behind,
+and the message says the recovery guard is busy and names both paths; when no `hack env`
+command is running, remove both and retry. `hack env hydrate` reports the same message
+when it cannot get the env file's lock.
 
 `.hack/.internal/` is ignored by the managed `.hack/.gitignore`, so lock files never show
 in `git status`. If `.hack/.internal` is a symlink or not a directory, these commands
@@ -926,7 +946,7 @@ it cannot spell portably as `<unprintable-path>/.hack.secret.key`.
 | `env-key-wrong-owner` | warn | the file is not owned by the current user |
 | `env-key-wrong-length` | warn | shorter than the 43-character base64url form of a 256-bit key, or empty |
 | `env-key-low-entropy` | warn | long enough but repetitive (estimated under 128 bits) |
-| `env-key-unignored` | warn | the checkout-local key is not ignored by git, so `git status` offers it for commit; suggests adding it to the root `.gitignore` |
+| `env-key-unignored` | warn | the checkout-local key is not ignored by git, so `git status` offers it for commit (for example after `git init` in a directory whose root `.gitignore` entry was removed); suggests adding it to the root `.gitignore` |
 | `env-key-symlink` | error | the key path is a symlink; Hack refuses symlinked keys |
 | `env-key-not-regular` | error | the key path is a directory or another non-file entry |
 | `env-key-unreadable` | error | unreadable, oversized or not UTF-8 |

src/commands/doctor.ts

@@ -2958,9 +2958,10 @@ async function checkComposeBackendFeatures(opts: {
 
 /**
  * Read-only key-strength and exposure audit (HACK-1127). `--fix` never acts on
- * it; each finding suggests the exact command instead.
+ * it; each finding suggests the exact command instead. It also catches a key
+ * that a later `git init` left unignored (`env-key-unignored`, HACK-1386).
  */
-async function checkProjectEnvKey({
+export async function checkProjectEnvKey({
   startDir,
 }: {
   readonly startDir: string;

src/lib/env-layer-lock.ts

@@ -13,18 +13,55 @@ import {
 export const ENV_LAYER_LOCK_WAIT_MS = 10_000;
 
 /**
- * Another `hack env` writer held the layer's lock for the whole bounded wait
- * (or an interrupted lock recovery needs offline inspection). Closed and
- * retryable: nothing was read or written, and the message names only the
- * layer path, never a key or value.
+ * Why {@link EnvLayerLockTimeoutError} gave up:
+ * - `held`: the lock stayed occupied for the whole wait. Either a live Hack
+ *   writer holds it, or Hack can never reclaim it on its own (a malformed
+ *   receipt, one owned by another user, or a PID now used by a live process).
+ * - `recovery_guard`: the owner has exited, but the `<lock>.recovery` guard
+ *   left by an interrupted reclaimer blocks reclamation until it is removed.
+ */
+export type EnvLayerLockTimeoutReason = "held" | "recovery_guard";
+
+/** The projects-registry lock's detail for a dead owner behind a busy guard. */
+const RECOVERY_GUARD_BUSY_DETAIL =
+  "recovery guard is busy (interrupted recovery requires offline inspection)";
+
+/**
+ * The layer's lock stayed held for the whole bounded wait. Closed and
+ * retryable: nothing was read or written. The one-line message names the
+ * layer, its lock path and the recovery step, never a key or value, so a lock
+ * Hack cannot reclaim does not leave the user retrying a command that can
+ * never succeed.
  */
 export class EnvLayerLockTimeoutError extends ClosedCliError {
-  constructor(opts: { readonly path: string; readonly waitMs: number }) {
-    super(
-      `Another hack env command is writing ${opts.path}; gave up after waiting ${formatWait(opts.waitMs)} for it. Nothing was written. Retry when it finishes.`
-    );
+  readonly reason: EnvLayerLockTimeoutReason;
+  readonly lockPath: string;
+
+  constructor(opts: {
+    readonly path: string;
+    readonly lockPath: string;
+    readonly waitMs: number;
+    readonly reason: EnvLayerLockTimeoutReason;
+  }) {
+    super(envLayerLockTimeoutMessage(opts));
     this.name = "EnvLayerLockTimeoutError";
+    this.reason = opts.reason;
+    this.lockPath = opts.lockPath;
+  }
+}
+
+function envLayerLockTimeoutMessage(opts: {
+  readonly path: string;
+  readonly lockPath: string;
+  readonly waitMs: number;
+  readonly reason: EnvLayerLockTimeoutReason;
+}): string {
+  const wait = formatWait(opts.waitMs);
+  if (opts.reason === "recovery_guard") {
+    const guard = `${opts.lockPath}.recovery`;
+    return `Refused to update ${opts.path}: its lock at ${opts.lockPath} was left by a process that has exited, and the ${RECOVERY_GUARD_BUSY_DETAIL} at ${guard}; gave up after waiting ${wait}. Nothing was written. If no hack env command is running, remove ${guard} and ${opts.lockPath}, then retry.`;
   }
+  return `Another hack env command is writing ${opts.path}; gave up after waiting ${wait} for its lock at ${opts.lockPath}. Nothing was written. Retry when it finishes. If no hack env command is running, Hack cannot reclaim that lock itself: remove ${opts.lockPath} and retry.`;
 }
 
 /**
@@ -69,7 +106,10 @@ export function resolveEnvLayerLockPath(opts: {
  *   reclaimed only after `kill(pid, 0)` reports the owner gone (ESRCH).
  * - The wait is bounded (`timeoutMs`, default {@link ENV_LAYER_LOCK_WAIT_MS})
  *   with jittered polling. When it runs out, {@link EnvLayerLockTimeoutError}
- *   is thrown and `run` never starts, so nothing is written.
+ *   is thrown and `run` never starts, so nothing is written. Its message
+ *   names the lock path and how to clear a lock Hack cannot reclaim itself
+ *   (a malformed or foreign receipt, a reused PID, an interrupted recovery's
+ *   guard); this helper never removes such a lock on its own.
  * - A symlinked or non-directory `.internal` beside the layer is refused with
  *   {@link EnvLayerLockRefusedError}; a filesystem failure while taking the
  *   lock (permission, full disk) is refused as the layer it blocked, never as
@@ -109,7 +149,14 @@ export async function withEnvLayerLock<T>(opts: {
       throw error;
     }
     if (error instanceof ProjectsRegistryLockTimeoutError) {
-      throw new EnvLayerLockTimeoutError({ path: opts.layerPath, waitMs });
+      throw new EnvLayerLockTimeoutError({
+        path: opts.layerPath,
+        lockPath,
+        waitMs,
+        reason: error.message.includes(RECOVERY_GUARD_BUSY_DETAIL)
+          ? "recovery_guard"
+          : "held",
+      });
     }
     if (!(await directoryWritable(dirname(opts.layerPath)))) {
       // No Hack writer can replace a layer whose directory it cannot write

src/lib/project-env-config.ts

@@ -4180,12 +4180,20 @@ async function writeProjectEnvKeyWithFallback(opts: {
   return { keyPath: opts.localKeyPath, keyText, warnings };
 }
 
-/** Fixed, value-free refusal for a checkout-local key git would commit. */
+/**
+ * Fixed, value-free refusal for a checkout-local key git would commit.
+ * `unwritable`: Hack could not add the entry to the root `.gitignore`.
+ * `reincluded`: the entry is there, but a later rule in that file (such as
+ * `!.hack.secret.key`) re-includes the key, so adding it again cannot help.
+ */
 function projectEnvKeyUnignoredError(opts: {
   readonly rootGitignore: string;
+  readonly cause: "unwritable" | "reincluded";
 }): ProjectEnvKeyRefusalError {
   return new ProjectEnvKeyRefusalError(
-    `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed, and Hack could not add it to ${opts.rootGitignore}. Add a '${PROJECT_ENV_KEY_FILENAME}' line to that file (making it writable if needed) and remove any rule that re-includes the key, then retry; key contents are never included in this message.`
+    opts.cause === "reincluded"
+      ? `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed. ${opts.rootGitignore} lists '${PROJECT_ENV_KEY_FILENAME}', but a later rule in that file re-includes the key (for example '!${PROJECT_ENV_KEY_FILENAME}'). Remove that rule, then retry; key contents are never included in this message.`
+      : `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed, and Hack could not add it to ${opts.rootGitignore}. Add a '${PROJECT_ENV_KEY_FILENAME}' line to that file (making it writable if needed) and remove any rule that re-includes the key, then retry; key contents are never included in this message.`
   );
 }
 
@@ -4203,7 +4211,12 @@ function projectEnvKeyUnignoredError(opts: {
  *   key is not used.
  *
  * Either way, a key git still does not ignore after the entry (for example a
- * later `!.hack.secret.key` rule) is refused.
+ * later `!.hack.secret.key` rule) is refused, naming that cause.
+ *
+ * A checkout outside git that later becomes one (`git init`) is not watched:
+ * Hack runs no code at that point. If the entry went missing meanwhile, the
+ * first `--secret` set or hydrate in the new checkout repairs it or refuses
+ * here, and `hack doctor`'s env key check reports `env-key-unignored`.
  */
 async function ensureProjectEnvKeyIgnored(opts: {
   readonly projectRoot: string;
@@ -4224,10 +4237,13 @@ async function ensureProjectEnvKeyIgnored(opts: {
   } catch (error: unknown) {
     throw opts.creating
       ? closedFileWriteError({ error, path: rootGitignore, operation: "write" })
-      : projectEnvKeyUnignoredError({ rootGitignore });
+      : projectEnvKeyUnignoredError({ rootGitignore, cause: "unwritable" });
   }
+  // The entry is in the root `.gitignore` now (written or already there), and
+  // no deeper ignore file applies to a key at the project root, so only a
+  // later rule in that same file can re-include it.
   if ((await isGitIgnoredProjectEnvPath(probe)) === false) {
-    throw projectEnvKeyUnignoredError({ rootGitignore });
+    throw projectEnvKeyUnignoredError({ rootGitignore, cause: "reincluded" });
   }
 }
 

tests/project-env-key-ignore.test.ts

@@ -11,13 +11,14 @@ import {
 } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join, resolve } from "node:path";
-
+import { checkProjectEnvKey } from "../src/commands/doctor.ts";
 import {
   PROJECT_COMPOSE_FILENAME,
   PROJECT_CONFIG_FILENAME,
   PROJECT_ENV_KEY_FILENAME,
 } from "../src/constants.ts";
 import { resolveProjectEnvConfigPath } from "../src/lib/project-env-config.ts";
+import { restoreEnv } from "./helpers/env.ts";
 import { privateCliEnv } from "./helpers/private-cli-env.ts";
 
 /**
@@ -233,6 +234,11 @@ function unignoredRefusal(repo: Repo): string {
   return `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed, and Hack could not add it to ${repo.gitignore}.`;
 }
 
+/** HACK-1386: the entry is present, so the refusal names the re-including rule instead. */
+function reincludedRefusal(repo: Repo): string {
+  return `Refusing to use ${PROJECT_ENV_KEY_FILENAME}: git does not ignore it, so it could be committed. ${repo.gitignore} lists '${PROJECT_ENV_KEY_FILENAME}', but a later rule in that file re-includes the key (for example '!${PROJECT_ENV_KEY_FILENAME}'). Remove that rule, then retry; key contents are never included in this message.`;
+}
+
 /** Creates the key with a first secret, then drops its ignore entry (the pre-fix state). */
 async function createUnignoredKey(repo: Repo): Promise<string> {
   const first = await setSecret(repo, "FIRST_SECRET", FIRST_VALUE);
@@ -352,17 +358,20 @@ test("a rule that re-includes the key is refused both before creating a key and
   await writeFile(repo.gitignore, reincluding);
 
   const first = await setSecret(repo, "NEW_SECRET", NEXT_VALUE);
-  expectOneLineRefusal(first, unignoredRefusal(repo));
+  expectOneLineRefusal(first, reincludedRefusal(repo));
+  expect(`${first.stdout}${first.stderr}`).not.toContain("could not add it");
   expect(await keyExists(repo)).toBe(false);
   expect(await readFile(repo.layer, "utf8")).toBe(LAYER);
+  // The file already listed the key, so it was not rewritten.
+  expect(await readFile(repo.gitignore, "utf8")).toBe(reincluding);
 
   // An existing key under the same rule is refused before it encrypts.
   await writeFile(repo.gitignore, BASE_GITIGNORE);
   const keyText = await createUnignoredKey(repo);
   await writeFile(repo.gitignore, reincluding);
   const layerBefore = await readFile(repo.layer, "utf8");
   const next = await setSecret(repo, "NEXT_SECRET", NEXT_VALUE);
-  expectOneLineRefusal(next, unignoredRefusal(repo));
+  expectOneLineRefusal(next, reincludedRefusal(repo));
   expectNoKeyMaterial(next, keyText);
   expect(await readFile(repo.layer, "utf8")).toBe(layerBefore);
 }, 60_000);
@@ -382,3 +391,88 @@ test("outside git an existing key is used without writing a root .gitignore", as
   expect(run.exitCode).toBe(0);
   expect(await lstat(repo.gitignore).catch(() => null)).toBeNull();
 }, 60_000);
+
+/**
+ * Runs `run` in this process with the fixture's private HOME, HACK_HOME and
+ * global config, and git's global, system and XDG configuration (so any
+ * personal excludes file) out of reach; restores the previous environment.
+ */
+async function withPrivateEnvironment<T>(
+  repo: Repo,
+  run: () => Promise<T>
+): Promise<T> {
+  const home = join(repo.root, "user-home");
+  const overrides: Record<string, string> = {
+    HOME: home,
+    XDG_CONFIG_HOME: join(home, ".config"),
+    HACK_HOME: join(repo.root, "hack-home"),
+    HACK_GLOBAL_CONFIG_PATH: join(repo.root, "hack-config.json"),
+    HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK: "1",
+    GIT_CONFIG_NOSYSTEM: "1",
+    GIT_CONFIG_GLOBAL: "/dev/null",
+  };
+  const previous = Object.fromEntries(
+    Object.keys(overrides).map((key) => [key, process.env[key]])
+  );
+  for (const [key, value] of Object.entries(overrides)) {
+    process.env[key] = value;
+  }
+  try {
+    return await run();
+  } finally {
+    for (const [key, value] of Object.entries(previous)) {
+      restoreEnv(key, value);
+    }
+  }
+}
+
+/** The real `hack doctor` env key check for the fixture, in this process. */
+async function doctorEnvKey(repo: Repo): Promise<{
+  readonly status: string;
+  readonly codes: readonly string[];
+  readonly message: string;
+}> {
+  const result = await withPrivateEnvironment(
+    repo,
+    async () => await checkProjectEnvKey({ startDir: repo.projectRoot })
+  );
+  return {
+    status: result.status,
+    codes: (result.envKey?.findings ?? []).map((finding) => finding.code),
+    message: result.message,
+  };
+}
+
+test("a key whose entry was removed before a later git init is flagged by doctor and repaired by the next secret set", async () => {
+  // HACK-1386: outside git an existing key needs no entry, and Hack does not
+  // observe a later `git init`. Doctor's env key check is the backstop, and
+  // the first secret write in the new checkout restores the entry.
+  const repo = await createRepo({ git: false });
+  const first = await setSecret(repo, "FIRST_SECRET", FIRST_VALUE);
+  expect(first.exitCode).toBe(0);
+  const keyText = (await readFile(repo.keyPath, "utf8")).trim();
+  await rm(repo.gitignore);
+  const second = await setSecret(repo, "NEXT_SECRET", NEXT_VALUE);
+  expect(second.exitCode).toBe(0);
+  expect(await lstat(repo.gitignore).catch(() => null)).toBeNull();
+
+  expect((await git(repo, ["init", "-q"])).exitCode).toBe(0);
+  expect(await keyIsCommittable(repo)).toBe(true);
+
+  const flagged = await doctorEnvKey(repo);
+  expect(flagged.status).toBe("warn");
+  expect(flagged.codes).toContain("env-key-unignored");
+  expect(flagged.message).toContain(
+    `env-key-unignored: ${PROJECT_ENV_KEY_FILENAME} is not ignored by git`
+  );
+  expect(flagged.message).not.toContain(keyText);
+  expect(flagged.message).not.toContain(repo.root);
+
+  const repair = await setSecret(repo, "THIRD_SECRET", FIRST_VALUE);
+  expect(repair.exitCode).toBe(0);
+  expectNoKeyMaterial(repair, keyText);
+  expect(await keyIsCommittable(repo)).toBe(false);
+  const repaired = await doctorEnvKey(repo);
+  expect(repaired.codes).not.toContain("env-key-unignored");
+  expect(repaired.status).toBe("ok");
+}, 60_000);