JavaScript GCP KMS Storage: close atomicWrite mutation-coverage gaps (KSM-1517)

[stas-schaller]

Summary

JavaScript GCP KMS Storage: closes 3 surviving mutation-testing gaps in the atomic-write hardening's own test coverage. No production code change.

Changes

Maintenance

• Re-ran mutation testing against the current suite. Two of the four originally-reported mutants (dropping O_EXCL on the temp-file open, dropping the directory-fsync call entirely) are now caught by GCPKeyValueStorage.directoryFsync.test.ts, added since the original report. Three still survive: the temp file's open mode can weaken from 0600 to 0644 unnoticed (masked by the redundant chmodSecure call later in the same function), the file's own data fsync can be dropped, and the directory's fsync can be dropped while the directory is still opened. Added test/atomicWrite.fsyncCoverage.test.ts targeting exactly those 3, using a chronological open/fsync/close event log scoped to each fd's own lifetime window rather than a bare fd-number match — fds are reused by the OS after close(), so a naive "was fsyncSync ever called with fd N" check can pass on a mutant that drops the earlier call, because a later, unrelated open reuses the same number. (KSM-1517)
• Tightened 3 bare .rejects.toThrow() assertions in test/GCPKeyValueStorage.zeroLengthConfig.test.ts to .rejects.toThrow(/is empty/), matching the pattern sibling assertions in the same file already use for the same error messages, so they can't pass for an unrelated rejection reason.

Testing

cd sdk/javascript/packages/gcp
npm test

127/127 passing (124 existing + 3 new). Each new test was verified against its own hand-applied mutation of atomicWrite.ts before being finalized: it fails when its targeted behavior is removed and passes otherwise, with no cross-contamination between the three.

Breaking Changes

None.

Related Issues

• Jira: KSM-1517

[mgallego-keeper]

Summary

This PR adds test/atomicWrite.fsyncCoverage.test.ts and tightens three assertions in test/GCPKeyValueStorage.zeroLengthConfig.test.ts. It makes no source code change.

Verification: the four named mutants are caught

The linked ticket names four mutations in src/atomicWrite.ts that the suite did not catch before this PR: the temp file's open mode (0o600 to 0o644), dropping the temp file's data fsyncSync call, dropping the fsyncDirectory call entirely, and dropping the wx flag on the temp file open.

I reverted each of the four, one at a time, in a merged copy of this PR, and ran the new test file each time. Each reversion produced a test failure. All four are caught.

I also checked a narrower case the PR body does not spell out: deleting only the inner fsyncSync(dirFd) call inside fsyncDirectory, while still opening and closing the directory. The older directoryFsync.test.ts test does not catch this, since it only checks that the directory was opened. This PR's new test does catch it. The new coverage is not redundant with the old test.

Finding: one of the four weak assertions the ticket names is still unfixed

The linked ticket, KSM-1517, names six assertions that cannot fail. Four are in test/GCPKeyValueStorage.zeroLengthConfig.test.ts, at lines 108, 126, 206, and 214. Two are in test/GCPKeyValueStorage.test.ts, at lines 458 and 467.

This PR tightens three of the four zero-length assertions, at lines 108, 126, and 214, to rejects.toThrow(/is empty/). It does not touch the fourth, at line 206, which still reads rejects.toThrow(configPath). It does not touch either of the two fs.writeFile assertions in test/GCPKeyValueStorage.test.ts.

I confirmed the line-206 gap directly. I deleted the zero-length guard in decryptConfig() in a merged copy of this PR, then ran test/GCPKeyValueStorage.zeroLengthConfig.test.ts alone. The two assertions this PR did tighten failed, as expected. The line-206 assertion still passed. The resulting error, from decryptConfig()'s own outer catch, is Failed to write decrypted config file <path>. That message still contains the config path, so the assertion cannot tell the two failure causes apart.

I also confirmed the two fs.writeFile assertions are still vacuous. No file under src/ calls fs.writeFile or fs.promises.writeFile anywhere. expect(fs.writeFile).not.toHaveBeenCalled() passes no matter what the code under test does.

Requested change

Tighten the line-206 assertion the same way as its three siblings, for example to rejects.toThrow(/is empty/). Delete the two fs.writeFile assertions in test/GCPKeyValueStorage.test.ts, or rewrite them against writeFileAtomicSync, which is the function the code actually calls now. The ticket's own "Test 5" and "Regression Checks" sections already call for exactly this.

Merge order

See the note on #1189. This PR has no file overlap with any of the other four.

[mgallego-keeper]

Summary

This PR adds test/atomicWrite.fsyncCoverage.test.ts and tightens several assertions in two existing test files. It changes no file under src/. I mutated the source in a scratch worktree, one change at a time, and reran the suite for each, restoring cleanly between each run.

Verification: both round-1 findings are fixed

Round 1 flagged two things. First, decryptConfig()'s "names the offending config file" test used only rejects.toThrow(configPath), which could not tell that error apart from a different, unrelated failure that also happens to mention the path. Second, two fs.writeFile-not-called assertions in GCPKeyValueStorage.test.ts were unconditionally true, because no code in this class calls fs.writeFile any more.

Both are fixed. I deleted decryptConfig()'s zero-length guard in a scratch copy and reran the suite: all three of that describe block's zero-length tests now fail, including the one round 1 named. I confirmed the two fs.writeFile assertions now spy on writeFileAtomicSync, the function this class actually calls, and that the spy mechanism works under this file's ts-jest compilation (a named import compiles to a property read on the shared module object here, so jest.spyOn on that object does intercept the call site).

Verification: the four named mutants in the linked ticket are all genuinely caught

I applied each of the four mutants named in KSM-1517 one at a time (temp file mode 0600 to 0644, dropping the temp file's data fsync, dropping the whole directory fsync call, losing O_EXCL by changing wx to w), plus the narrower case from round 1 (dropping only the inner directory fsync call while still opening and closing the directory). Each one is caught, each by exactly the test that should catch it, with no unrelated tests affected. I also confirmed the new mode test measures the temp file's own open call, not the later chmodSecure call, by deleting that later call and confirming the new test still passes on its own. The headline mutation (replacing the whole writeFileAtomicSync body with a plain fs.writeFileSync) is still caught, now by 11 tests across 5 files.

Finding: one claimed regression test does not test what its commit message says

The commit for the fs.writeFile to writeFileAtomicSync change says a mutant that widens createConfigFileIfMissing()'s ENOENT-only check to also swallow EACCES makes "both replaced assertions... fail; the originals could not have caught it." I reproduced that exact mutant. Both of the two affected tests do fail, but not on the new spy assertion. Each fails one line earlier, on the pre-existing rejects.toMatchObject({ code }) assertion, because this describe block's crypto client mock has no default return value for encrypt(), so any path that reaches the placeholder-config write crashes with an unrelated TypeError before the spy assertion ever runs. The fix is still the right one (it spies on the function this class actually calls), but for this describe block it currently adds no independent detection power of its own. Not a blocker. Worth knowing before relying on that commit message's claim elsewhere.

Finding: two gaps remain in exactly the function this PR targets

I mutated atomicWrite.ts two more ways, beyond the four named in the ticket. Moving the directory fsync to before the rename instead of after leaves the entire suite (18 of 18) passing. Changing fsyncDirectory() to open the renamed file itself instead of its containing directory also leaves the entire suite passing; nothing in any test, old or new, checks which path got opened, only that something was opened with a read flag and fsynced.

Both are one-line-sized future mistakes that this PR's own new test file, by its own stated purpose, would be expected to catch and currently does not. Today's shipped code is correct on both counts, so this is not a blocker. Filed as KSM-1543, linked to this ticket, rather than expanding this PR further.

Smaller notes, non-blocking

• test/atomicWrite.fsyncCoverage.test.ts's directory-fsync test has no Windows guard, unlike a sibling test later in the same file family. Not new: the pre-existing directoryFsync.test.ts has the identical gap already. No practical effect today, since this workflow only runs on ubuntu-latest.
• The linked ticket's own "Test 4" asks for a behavioral test of the O_EXCL protection: a real pre-existing file at the predicted temp path, left untouched after the write fails with EEXIST. I confirmed no test anywhere in the suite does this; the current coverage only checks the open call's flag string.
• The PR body's numbers are stale relative to the current branch tip ("Tightened 3" assertions, "127/127" tests). Not a defect, just worth a refresh before merge.

Recommendation

Approve. The fixes for both round-1 findings hold up under actual mutation, and the four ticket-named gaps are genuinely closed. The two new gaps above are narrow and do not regress anything shipped today, so KSM-1543 covers them instead of blocking this PR.