Skip to content

fileLock key derivation guesses the link parent when realpath fails, and the two derivations can pick different locks #2404

Open

Description

@easonLiangWorldedtech

Where this was measured. Branch vps2f-1914-u5a at 3901239982bc8fa3ad96996539b4845aaa25bb31 (PR #2359) and branch vps2f-1914-u5b at d6fa7117e07b70455da24e4722b47eb0b202fa6e (PR #2357). src/utils/fileLock.ts is the same blob at both heads: git rev-parse <rev>:src/utils/fileLock.ts gives 0705f771ccac98167f71562ed418da4106b8b31d at each, 241 numbered lines, so every line below reads the same at either head.

The finding. Two key derivations fall back to a guess where the block next to them fails closed:

  • src/utils/fileLock.ts:71-74 canonicalizes a relative link target's parent with await fs.realpath(path.dirname(linkCursor)).catch(() => undefined) and then joins path.resolve(parentReal ?? path.dirname(linkCursor), referent). Every failure of that realpath (EACCES on the parent, EIO, ELOOP) becomes undefined, and the lexical join then names a lock for a different file. The comment at :66-70 states the intent, that a relative referent is relative to the link's real parent; the code makes honoring it conditional on the parent being inspectable.
  • The block immediately after it applies the opposite rule. src/utils/fileLock.ts:85-103 walks the missing-path chain and rethrows anything that is not ENOENT or ENOTDIR (:99), and the comment at :94-98 says in words that an uninspectable ancestor means the canonical key is unknown and that a lexical key is a different lock from the one a peer that reached the file through the referent holds. The link-parent path does not get that treatment.
  • The writer's own derivation diverges on the same input. src/services/file-safety/safeWriteText.ts:445-469 (resolveLockKey) joins a relative publish target lexically at :467, key = await canonicalDirKey(path.resolve(path.dirname(key), target)), with no parent realpath at all, so one caller can key the file by its alias directory while another keys it by the referent directory. src/utils/safeWriteJson.ts:248, :305 and :319 call that function for acquisition, the recheck under the lock, and the release guard.

Reviewer asks this records. Thread 4509350117 (coderabbitai[bot], submitted 2026-11-09T04:06:10Z, last edited 2026-11-09T07:47:26Z) at src/utils/fileLock.ts:74 carries two asks: "Propagate unexpected link-parent resolution errors." and "Use the real parent in resolveLockKey." Its guidance line reads: "Retain fallback behavior only for failures established as safe to recover from, so both key checks cannot silently select an unrelated lock path." In row 4510565148 (coderabbitai[bot], submitted 2026-11-09T08:04:01Z) the reviewer offered: "Would you like me to open a follow-up issue for the canonical-keying split, linked to this PR and thread?" This issue is that offer accepted, so the finding lives somewhere that survives review collapsing.

What is not claimed. No measured production incident, no claim that a named caller hits the divergence today, and no severity beyond what the reviewer recorded (Minor, Stability and Availability, Quick win). The claim is only what the two derivations do when a parent cannot be inspected.

Where it relates. #2357 and #2359 both carry the file and both carry the thread. The fix belongs to #2393 (fix(utils): one file, one lock artefact spelling, one canonical key, head 2c79cd47d866), which is the unit that owns src/utils/fileLock.ts and its three specs.

Price status. Unpriced. A fix changes executable lines in fileLock.ts and in safeWriteText.ts. Neither chain head can absorb it: the gate's own selector (scripts/stryker-diff.mjs, blob 7dfe0006d2d5c7e4d2282f65f68b8ed5f16866bc) reports "extension has 770 changed executable lines (limit 500)" for #2357 at d6fa7117e07b70455da24e4722b47eb0b202fa6e, and "extension has 1284 changed executable lines (limit 500)" for #2359 at 3901239982bc8fa3ad96996539b4845aaa25bb31 and again at ebd6d9e17df3bc0d3d234bb13dbe00b5be5be3bf, on the branch leg (base 5319fe91fb0caf4aa4ec66aac68fa03744ebbc23) and on the CI leg (merge-tree of the head with tip 18861b2e357203e6cb4d93930aa2ed122b1eecea, priced against the resulting synthetic merge commit). So this travels with #2393 rather than onto either chain head.

Found while answering CodeRabbit threads on #2357 and #2359. Filed on the ruling that a finding recorded only inside a review reply gets collapsed, marked outdated and re-scored, while an issue does not.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions