Skip to content

safeWriteText reports post-commit failures only through the optional onPostCommitWarning callback #2402

Open

Description

@easonLiangWorldedtech

Where this was measured. Branch vps2f-2357-u1-writer, head 20182be34777812b2aaa8c0b21d47536d926dae1 (PR #2385), file src/services/file-safety/safeWriteText.ts, 1,236 lines at that head. Every line number below is read from that tree.

The finding. Two known post-commit failures have a reporting path, and that path is optional, so a caller that passes no listener gets a resolved write and no signal:

  • :1067 const stagingResidue = await fs.unlink(tempPath).catch((error: unknown) => error), then :1068-1074 reports it as options?.onPostCommitWarning?.({ kind: "staging-link-orphan", path: tempPath, cause: stagingResidue }). On the hard-link commit path the target name and the staging name are the same inode until that removal succeeds, so a leftover tempPath is a second link to content the target already publishes, and a caller that reuses the path without knowing the removal failed can truncate it. The comment at :1060-1066 states the intent, that the residue must be named rather than dropped, and the code makes the naming conditional on the caller supplying a listener.
  • :1108-1118 reports a failed DACL restore as options?.onPostCommitWarning?.({ kind: "dacl-restore-failed", path: restoredDir, cause: restoreFailure }), where restoredDir is path.dirname(targetPath) at :1109. With no listener, a failure to put a directory DACL back is reported to nobody, and the payload names the directory whose entry the restore rewrites rather than the target file.
  • The payload union is declared at :97 (kind: "staging-link-orphan" | "dacl-restore-failed"), so those two are the whole channel; there is no logger, no throw and no return field carrying them.

What is not claimed. No severity, no measured production incident, no claim that a named caller hits this today. The claim is only what the code does.

Where it relates. #2390 (the staging hard link left after fs.link and its best-effort cleanup) and #2392 (the wider Windows DACL work, including applying the target DACL to staging and backup files before they receive content). This is the reporting half of both, which neither issue states.

Price status. Unpriced. Any fix changes executable lines. #2385 is frozen at 526 counted executable lines against the gate cap of 500 (job 129791888819, log line 1884 at 2026-11-10T11:09:22.585Z) pending a maintainer decision on a narrow exclusion or a split, so this travels with the follow-up branch that carries it, priced by the CI-leg method (git merge-tree of the branch head with main, then scripts/stryker-diff.mjs selectFromGit against the resulting synthetic merge commit), not onto the frozen PR.

Found while answering CodeRabbit threads on #2385: reviewer rows 4524154014 and 4524154958 raised the two halves, and author answers 4524182322, 4524182605 and 4524183447 carry the line-level evidence. 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