fix: replace node-persist with minimal in-repo file storage

[tim-fin]

Closes #1107 — implements the scope agreed there.

What

• Adds HAPFileStorage (~100 lines of fs), implementing exactly the four operations HAP-NodeJS uses — initSync, getItem, setItemSync, removeItemSync — with the same synchronous semantics and the same on-disk layout as node-persist@0.0.12: raw-key filenames, bare JSON.stringify contents, values loaded once at init and served from memory afterwards.
• HAPStorage.storage() now returns it. AccessoryInfo, IdentifierCache and ControllerStorage are untouched — no call site changes shape.
• Every other method of node-persist's LocalStorage prototype (all 36 of them) is stubbed at runtime to throw with a message naming the method, the four supported operations, and a link to Update node-persist dependency to remove deprecated q package #1107 — so a plugin depending on more gets a clear error instead of undefined behaviour. TypeScript consumers get a compile error instead, since the stubs are deliberately absent from the class type.
• Fixes a latent data-loss bug in passing: node-persist decided whether a path was absolute with normalize(dir) !== resolve(dir), which misclassified absolute paths carrying a trailing separator and wrote that data into node_modules/node-persist/src/storage/, where the next npm install wiped it. The new check is path.isAbsolute(), so those paths now store where the caller asked.
• Anything inside the storage directory which is not a regular file — a directory, or a symlink pointing at one — is skipped when loading, rather than aborting startup with a bare EISDIR.
• Keys are required to be plain filenames, so one file per key inside the storage directory is an invariant rather than an intention.
• Removes node-persist (and with it q and mkdirp@0.5.x), the hand-written src/types/node-persist.d.ts, the __mocks__/node-persist.ts jest automock, and the .github/node-persist-ignore.js build workaround, which only existed to paper over node-persist type collisions.
• The behavior contract (layout guarantees, supported operations, loud-failure semantics) is documented as typedoc on the exported class and interface, so it lands on the generated docs site with the next release.

Verification

• 52 unit tests on the storage class, including byte-level assertions on written files.
• A differential harness run locally: identical operation sequences through real node-persist@0.0.12 and through HAPFileStorage — directory listings and every file byte-identical, each implementation cleanly reads directories written by the other, identical dotfile and corrupt-JSON handling, identical reference/caching semantics (ControllerStorage.load() relies on mutating the object getItem returns), identical error ordering on fs failures. Happy to attach the script if useful.
• Full suite: 870 tests passing (was 817), lint and typedoc clean.
• Jest now redirects the storage singleton into a per-suite temp directory (jest.setup.ts, removed again in afterAll), so the suites that exercise publish() (Accessory.spec, HAPServer.spec) persist for real instead of into an automock — they pass unchanged against the real implementation.
• Homebridge core itself only ever calls HAPStorage.setCustomStoragePath(), always with an absolute path, so the main consumer sits comfortably inside the supported surface.

Deliberate divergences, flagged for your veto

1. A relative or non-string custom storage path now throws. node-persist silently redirected relative paths into its own module directory inside node_modules, where the data was lost on the next npm install; dir: undefined crashed with an unhelpful TypeError. Neither seemed worth preserving, so both fail loudly with a clear message. One subtlety the other way: node-persist's absoluteness check (normalize !== resolve) also misclassified absolute paths carrying a trailing separator and buried those in node_modules too — the new check is path.isAbsolute(), so such paths now work as the user intended. Absolute paths as Homebridge passes them behave exactly as before.
2. Unsupported init options (ttl, interval, custom stringify/parse, …) throw instead of being ignored. Supporting them would mean reimplementing most of node-persist; silently accepting-but-ignoring them seemed worse than refusing.
3. node-persist's legacy getItem(key, callback) form throws. It did work before (0.0.12 invoked the callback synchronously in addition to returning the value), but supporting it would keep two calling conventions alive for one method; refusing loudly keeps the surface at exactly four operations, one form each. Easy to change to a 3-line compatibility shim if you'd rather keep it working.
4. A key which is not a plain filename now throws. node-persist read keys as paths, so nested/key.json created a subdirectory and ../escaped.json wrote outside the storage directory entirely. A key starting with a dot is refused for the same reason, since dotfiles are skipped on load. Each of those would have written successfully and then come back undefined after a restart — a silent loss, where 0.0.12 at least crashed with EISDIR on the nested case. Nothing in HAP-NodeJS can produce such a key (AccessoryInfo, IdentifierCache and ControllerStorage all build <Type>.<MAC>.json), so this is unreachable in practice today; rejecting it keeps the layout guarantee honest for anyone building on the class later.

Writes are atomic, which node-persist's were not: each value is written to a temporary file which is then renamed over the target, so an interrupted write leaves the previous file intact instead of a truncated one — a truncated AccessoryInfo reads back as an accessory that has lost its pairings. The temporary file is named as a dotfile so a leftover from a crash is never loaded as a key, and carries the pid so processes sharing a storage directory (child bridges) can't race on it. The in-memory value is replaced only once the write has reached the disk, so a failed write cannot leave memory serving a value which would be gone after a restart.

Naming (HAPFileStorage), export placement, and anything else are of course yours to bikeshed — happy to adjust.

[Copilot]

Pull request overview

This PR replaces the deprecated node-persist@0.0.12 dependency with a minimal in-repo synchronous file storage implementation (HAPFileStorage) that preserves the on-disk format and the small API surface HAP-NodeJS relies on, while simplifying the dependency graph and test setup.

Changes:

• Introduces HAPFileStorage (init/load in-memory cache + getItem/setItemSync/removeItemSync) and switches HAPStorage to use it.
• Removes node-persist (and related types, mocks, and build workaround) from dependencies and the codebase.
• Updates tests to exercise real persistence via a Jest temp directory setup and adds dedicated HAPFileStorage unit tests.

Reviewed changes

Copilot reviewed 14 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
typedoc.config.mjs	Stops treating LocalStorage as intentionally unexported now that node-persist is removed.
src/types/node-persist.d.ts	Removes the hand-written node-persist type shim.
src/lib/model/IdentifierCache.spec.ts	Updates tests to assert persisted data via real storage instead of mocked LocalStorage.
src/lib/model/HAPStorage.ts	Switches singleton storage from node-persist to HAPFileStorage.
src/lib/model/HAPStorage.spec.ts	Updates storage tests to validate real filesystem behavior via temp dirs.
src/lib/model/HAPFileStorage.ts	Adds the new minimal storage implementation and runtime stubs for removed methods.
src/lib/model/HAPFileStorage.spec.ts	Adds focused unit tests covering layout compatibility and supported/unsupported operations.
src/lib/model/AccessoryInfo.ts	Updates a comment to avoid referencing node-persist directly.
src/lib/model/AccessoryInfo.spec.ts	Updates test to seed storage via setItemSync instead of mocking getItem.
src/index.ts	Exports HAPFileStorage from the public entrypoint.
package.json	Removes node-persist dependency and drops the build workaround script step.
package-lock.json	Removes node-persist, q, and mkdirp@0.5.x from the lockfile.
jest.setup.ts	Adds per-test-file temp storage redirection for the HAPStorage singleton.
jest.config.ts	Registers jest.setup.ts via setupFilesAfterEnv.
.github/node-persist-ignore.js	Deletes the now-unneeded build workaround script.
mocks/node-persist.ts	Deletes the Jest automock for node-persist.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/lib/model/HAPFileStorage.ts:118

• setItemSync/removeItemSync currently treat the key as a path (path.join(dir, key)) and even mkdirSync(path.dirname(file)). That allows path traversal (e.g. key = "../x") and can create nested subdirectories; on the next initSync() those subdirectories will make readFileSync throw unless handled. To keep the stated "one file per key inside the storage directory" invariant (and avoid writing outside dir), validate keys are simple filenames and avoid creating per-key directories.

    const file = path.join(this.dir, key);
    fs.mkdirSync(path.dirname(file), { recursive: true });
    fs.writeFileSync(file, JSON.stringify(value));

src/lib/model/HAPFileStorage.ts:92

• initSync() calls readFileSync on every non-dot entry from readdirSync(dir). If the storage directory contains a subdirectory (or other non-file), this will throw (e.g. EISDIR) and prevent storage initialization. Skipping non-regular files keeps init resilient and avoids a state where a key containing a path separator breaks future loads.

This issue also appears on line 116 of the same file.

      for (const filename of fs.readdirSync(this.dir)) {
        if (filename.startsWith(".")) {
          continue;
        }

[bwp91]

Sorry for the slow reply on this one — it deserved a proper look rather than a skim.

I went through it fairly carefully, and rather than just trusting the write-up I re-ran a lot of it myself: the full test suite, plus a side-by-side comparison against the real node-persist covering good files, truncated ones, empty ones, dotfiles, writes, deletes and a few odd values like undefined and circular objects. Everything came back identical. I also checked the caching behaviour, since child bridges are separate processes sharing the same folder, and that matches too. So the drop-in claim holds up.

One thing you've been too modest about. You mention that node-persist mishandled absolute paths with a trailing slash — what it actually did was dump the data inside node_modules/node-persist/..., where it would be wiped by the next npm install. So you've quietly fixed a real data-loss bug there. That's worth shouting about, not burying in a footnote.

On the three things you flagged for a veto: all fine by me. Relative paths can't reach us anyway — Homebridge always resolves the storage path to an absolute one before it gets that far — and I agree that failing loudly beats silently ignoring things.

Two things I'd like changing before it goes in:

1. Make the writes atomic — write to a temp file, then rename. I know you deliberately left this out to keep the change tight, and I take the point that it's a pre-existing issue rather than one you've introduced. But this is the file that holds pairing data, and a half-written file reads back as empty, which looks to the user like their accessory has simply unpaired itself. Since you're already touching every write, it's much cheaper to do now than to come back to.

2. Ignore folders when loading. If anything ever creates a directory inside the persist folder, startup dies with a cryptic EISDIR. node-persist did the same so it's nothing you've broken, but you're rewriting that bit anyway and it's a one-liner.

Nothing else in the code, as far as I can tell.

Two other things, not code:

This is a breaking change for any plugin using the bits you've removed, so it can't go out as a patch. It also isn't going into 2.1.10 — that release is already on the critical path for Homebridge 2.x going stable and I don't want to bolt a storage rewrite onto it. I'll aim it at 2.2.0. I'll also make sure the release notes spell out which methods have gone, so plugin authors can search for them rather than finding out the hard way.

Last thing — the CI here hasn't actually run yet, it's waiting on approval because it's your first PR to this repo. I'll kick that off so we get a proper result across all the Node versions rather than just my laptop.

Genuinely good work, and thanks for the detail in the description — the documentation on the class and making the removed methods fail with a clear message rather than just vanishing were both the right calls.

[tim-fin]

Both changes are in, c53708a3 plus a follow-up test in f771b758.

Atomic writes. setItemSync writes to a temporary file and renames it over the target. Two details are choices rather than mechanics, so worth flagging:

• the temporary file is a dotfile (.AccessoryInfo.XX.json.<pid>.tmp), so if a crash ever leaves one behind, initSync skips it instead of loading it as a key
• it carries the pid, so two processes sharing a storage directory — the child bridge case you raised — can't land on the same temporary path

If the rename fails, the temporary file is removed and the original error rethrown, so a failed write doesn't leave litter in the user's persist folder. The final bytes on disk are unchanged, and a write onto an occupied path still fails with EISDIR — I checked that one both ways, since the mechanism changed underneath it.

Directories on load. readdirSync now uses withFileTypes and skips directories. I only skip directories rather than everything that isn't a regular file: a symlink pointing at a real file still loads, which is what node-persist did, and silently ignoring one would be a regression for anyone relying on it.

On the testing, since this is pairing data and "it passes" felt like a thin claim for a durability change:

• The storage class is up to 38 tests (855 in the suite), including the circular-object case from your run and a check that a failed write leaves the previous contents intact.
• I measured the property rather than assuming it: four processes hammering the same key with a ~300KB value while a reader reads that file in a loop. With node-persist's in-place writeFileSync, the reader got truncated, unparseable JSON in 255 of 339 reads. With the temp-file-and-rename version, 0 of 521, and the file was still valid at the end. So the failure mode you described is straightforwardly reproducible, and this does close it.
• I also checked the new tests actually fail when the behaviour is removed — backing out the cleanup unlink fails the litter test, and a directory in the persist folder does throw EISDIR through readFileSync without the skip. Didn't want to ship assertions that pass vacuously.
• One that turned out to be parity rather than a divergence: I'd added mkdirSync(path.dirname(file)) for nested keys, and checked 0.0.12 did the same (mkdirp.sync at local-storage.js:475) rather than leaving it as an unflagged difference. f771b758 pins it, along with the temporary file landing beside its target — if it didn't, the rename wouldn't be atomic.

Two limits worth putting on the record rather than leaving as surprises. Rename gives an atomic replace, but nothing is fsynced, so a hard power cut can still lose the most recent write on some filesystems — it just can't corrupt the file that was already there; happy to add the fsync if you'd like that closed too, though it costs on every write. And I can't test Windows here: rename over an existing file is atomic there via MoveFileEx, but it can fail with EPERM if another process has the target open, where an in-place write would have succeeded — CI is Linux-only so it won't surface there either.

2.2.0 and spelling the removed methods out in the release notes both make sense. If it saves you a step, I can paste the full list of the 36 names in a comment for you to lift.

I've also updated the description: the trailing-separator/node_modules data loss is in the summary now rather than a footnote, and the paragraph claiming writes were left non-atomic is gone, since it no longer applies.

[bwp91]

Hey!

One small thing on this test, and it's a question rather than a request.

I can see the real point of that test is the second assertion — that the temp file lands beside its target rather than in the storage root, since the rename wouldn't be atomic otherwise. That's well worth pinning and I'd keep it.

But the title says nested keys work "like node-persist 0.0.12", but with directories now skipped on load, a key like nested/key.json writes fine and then comes back undefined after a restart — the file is there, initSync just never looks in the subdirectory. node-persist actually crashed with EISDIR in that situation, so we've gone from a loud failure to a silent one, which for pairing data I'd rather not have.

Nothing in HAP uses keys with a separator, so this is theoretical today. But the test reads as though nested keys are supported, and someone may well build on that later.

Worth tightening? Two options I can see: reject a key containing a path separator, which fits the four-operations-and-fail-loudly approach you've taken elsewhere, or keep the test purely as the atomicity check and drop the nested-key claim from its name. I don't have a strong preference — you've been closer to this than I have.

Thanks!

[tim-fin]

Confirmed, and it's worse than that test made it look — I reproduced it before changing anything. A key with a separator writes fine and then reads back undefined after a restart, exactly as you describe. path.join also means ../escaped.json writes outside the storage directory altogether; Copilot flagged that one at low confidence and I'd left it alone.

So I've taken your first option. setItemSync and removeItemSync now reject any key which isn't a plain filename — separator either way, ., .., empty. That closes both, and it turns the "one file per key inside the storage directory" line in the class doc into an invariant rather than an intention.

It is a fourth deliberate divergence, so it's in the description with the others for you to veto. Nothing in HAP-NodeJS can reach it — AccessoryInfo, IdentifierCache and ControllerStorage all build <Type>.<MAC>.json — so it's about what someone builds on the class later, which is the case you raised.

The assertion you wanted kept is still pinned, just no longer via nesting: the test spies on renameSync and asserts the temporary file and its target share a directory, which is the property that actually matters. I kept the mkdirSync before each write too — with nesting gone its only remaining job is recreating a storage directory removed at runtime, which 0.0.12 did through mkdirp.sync, so there's a test on that as well.

Storage class is at 46 tests, 864 in the suite, and I checked the new ones fail when the guard is taken back out.

Unrelated, from the run you kicked off: the single Coveralls regression was mine — HAPStorage.ts:30, the default-persist branch, which nothing reached once the node-persist automock was gone. 39c42e4d covers it and the file is back to 100%. Worth knowing that each push re-arms the first-PR approval gate, so the build is waiting on you again.

[coveralls]

Coverage Report for CI Build 30512440035

Coverage increased (+0.2%) to 66.405%

Details

• Coverage increased (+0.2%) from the base build.
• Patch coverage: 48 of 48 lines across 3 files are fully covered (100%).
• 1 coverage regression across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
src/lib/model/HAPStorage.ts	1	90.0%

---

Coverage Stats

Relevant Lines:	9599
Covered Lines:	6696
Line Coverage:	69.76%
Relevant Branches:	3415
Covered Branches:	1946
Branch Coverage:	56.98%
Branches in Coverage %:	Yes
Coverage Strength:	334.53 hits per line

---

💛 - Coveralls

[tim-fin]

Replying here rather than in the thread — the fix replaced the test your comment was anchored to, so GitHub has collapsed it as outdated.

Confirmed, and it's worse than that test made it look — I reproduced it before changing anything. A key with a separator writes fine and then reads back undefined after a restart, exactly as you describe. path.join also means ../escaped.json writes outside the storage directory altogether; Copilot flagged that one at low confidence and I'd left it alone.

So I've taken your first option. setItemSync and removeItemSync now reject any key which isn't a plain filename — separator either way, ., .., empty. That closes both, and it turns the "one file per key inside the storage directory" line in the class doc into an invariant rather than an intention.

It is a fourth deliberate divergence, so it's in the description with the others for you to veto. Nothing in HAP-NodeJS can reach it — AccessoryInfo, IdentifierCache and ControllerStorage all build <Type>.<MAC>.json — so it's about what someone builds on the class later, which is the case you raised.

The assertion you wanted kept is still pinned, just no longer via nesting: the test spies on renameSync and asserts the temporary file and its target share a directory, which is the property that actually matters. I kept the mkdirSync before each write too — with nesting gone its only remaining job is recreating a storage directory removed at runtime, which 0.0.12 did through mkdirp.sync, so there's a test on that as well.

Storage class is at 46 tests, 864 in the suite, and I checked the new ones fail when the guard is taken back out.

Unrelated, from the run you kicked off: the single Coveralls regression was mine — HAPStorage.ts:30, the default-persist branch, which nothing reached once the node-persist automock was gone. 39c42e4d covers it and the file is back to 100%. Worth knowing that each push re-arms the first-PR approval gate, so the build is waiting on you again.

[bwp91]

Had another proper look through this, and re-ran the whole suite on your branch rather than going off the description — 864 passing, lint clean.

The concurrency measurement is the bit that stands out. 255 truncated reads out of 339 before, 0 out of 521 after, is exactly the kind of evidence I wanted and did not ask for. Checking your new tests actually fail when you back the behaviour out is also more than most PRs bother with, and it is the reason I am happy to trust the rest of it.

I have left three comments on the code. One of them I think genuinely matters, the other two are smaller.

The honest summary: the first one is the same bug you just fixed, reached a different way. Your key guard closes the nested/key.json door but leaves the one next to it open, and I only spotted it because you had just taught me what to look for.

Two things that are mine to decide, not yours:

fsync — you are right about what rename does and does not give us. I am going to say leave it. This is pairing data, but it is also every write, and the thing that was actually hurting people was corruption rather than losing the last write to a power cut. If someone reports the power-cut case we can revisit it with a real report behind it.

Windows EPERM — nothing to do here, but thank you for writing it down rather than letting us find out from an issue. I have made a note so it is not a surprise later.

And no need to paste the 36 names — UNSUPPORTED_METHODS in the source already is that list, so I will lift it straight from there for the release notes.

These are getting smaller each round and we are definitely getting there. Sort the key guard and I am happy; the other two are your call.

[bwp91]

This is the one I would like changed.

The guard stops /, \, ., .. and empty, but not a key that merely starts with a dot. And initSync above skips dotfiles on purpose, so:

storage.setItemSync(".hidden", { paired: true })
storage.getItem(".hidden")   // { paired: true }  - looks fine
// restart
storage.getItem(".hidden")   // undefined

I ran it to be sure rather than reading it off the screen, and that is what happens — writes happily, comes back undefined after a restart.

Which is the exact thing you just fixed for nested/key.json, only arriving from a different direction. Same silent loss, same "looks like it worked" while it is running.

Same reasoning as before applies too: nothing in HAP-NodeJS can reach it, since everything here builds <Type>.<MAC>.json. So it is about whatever gets built on this class later, which is the case I raised in the first place.

Should just be adding a leading dot to what you already reject.

[bwp91]

Small one, and arguably too obscure to care about — your call entirely.

entry.isDirectory() does not follow symlinks, so a symlink pointing at a directory returns false here, gets past the skip, and lands in readFileSync below:

EISDIR: illegal operation on a directory, read

Which is the crash this line exists to prevent. I made one and confirmed it.

I would not raise it at all except you clearly thought about symlinks already — skipping only directories so a symlink to a real file still loads was the right call, and this is just the other half of the same thought. Something like entry.isFile() || (entry.isSymbolicLink() && fs.statSync(...).isFile()) keeps the behaviour you wanted.

Happy for you to wave this one off as not worth the code.

[bwp91]

This lands before anything touches the disk, so if the write or the rename throws, memory and disk disagree:

disk    { v: 1 }   <- correctly kept, your atomic write did its job
memory  { v: 2 }   <- the value that never actually got saved

Everything reads from memory, so the process carries on as if the write worked. For an AccessoryInfo that reads as paired now and unpaired after a restart, which is the confusing version of the problem you set out to fix rather than the loud one.

Your test covers the disk side and passes — it is only the in-memory half that is not pinned.

Moving this below renameSync should do it, so memory only ever holds what actually reached the disk.

[tim-fin]

All three are in cfc50c0d. Replying here rather than in the threads — the changes moved every line they were anchored to, so GitHub has collapsed all three as outdated.

Leading dot. Reproduced it before changing anything, and it behaves exactly as you ran it: writes, serves from memory for as long as the process lives, gone after a restart. The guard now rejects any key starting with a dot, which subsumes the . and .. cases it was checking for by name. Flagged in divergence 4 in the description with the others.

Symlink. Rather than adding a symlink branch to the dirent check, initSync now stats the entry and skips anything which is not a regular file. Same outcome for your case, and it also covers a broken or circular link, where the stat throws rather than reporting the wrong type — skipped rather than aborting startup, which is the same reasoning. A link to a real file still loads. Three tests, skipped automatically where creating a symlink is not permitted, since Windows wants Developer Mode for that.

Memory ahead of disk. this.data.set now runs after the rename. You are right that the existing test only pinned the disk side, so there are two more: an overwrite whose write fails, which must leave the previous value in memory, and a first write which fails, after which the key must not be in memory at all.

Storage class is at 52 tests, 870 in the suite, lint and typedoc clean. Each fix fails when the behaviour is backed out — one test for the dot, two each for the other two.

Noted on fsync and on the Windows EPERM. And UNSUPPORTED_METHODS is all yours for the notes.