create: keep ids server-owned

[aberoham]

NicTool/validate#34 forbids id on create, and validate 1.0.1 ships it, so this bumps the dependency. Every HTTP POST now leaves id allocation to the store. mysql uses its auto-increment columns. The json and toml stores serialize create allocation per file in the API process. A high-water id prevents reuse after the highest row is deleted. Allocation fails when the backend id type is exhausted.

Internal creates with explicit ids keep #69's collision rejection. HTTP creates refuse an id for every object: group, nameserver, permission, user, zone and zone record. Creates without an id still allocate one. PUT keeps using the id in the route.

A regression runs twenty concurrent group creates on json and toml. Each gets ids 1 through 20. After id 20 is destroyed, the next create gets 21.

Coverage now measures the json store as well as mysql. It goes through each backend's serial runner, because test files running in parallel rewrote the same store files.

This completes the last open API #6 item, caller-controlled auto-increment fields. Its permissions checkbox is still unticked, but #57 already implemented it.

Every file-store write now runs under that per-file lock: create, put, delete, destroy and the login hash upgrade. Each used to save a list it had loaded earlier, so rows created in between vanished. Regressions force each interleaving on json and toml. A write goes to a temp file renamed over the store file, so an unlocked load never reads half of one. A permission with no user or group answers 400 on every store.

[msimerson]

I'm confined to a phone screen today, but if you checkout the subproject '.release" in the validate repo, and follow the README steps, it'll create a release PR which I can merge and will auto-release.

[aberoham]

validate v1.0.1 is published. ef3536e removes the temporary source install and requires @nictool/validate ^1.0.1.

Local results: MySQL passed 418/418 in normal and coverage modes. JSON and TOML passed 395/395 each. Prettier and the workspace hook passed. ESLint has no errors.

[msimerson]

This is still a draft. Since you're building on it in another PR, perhaps come back and finish it up?

[msimerson]

The two new fixes look right. Permission create now goes through mutate(), and so does the login hash upgrade, which also skips the save if the stored hash changed while hashing. The race tests are reliable now that they inject the competing creates after the store has read the file.

Copilot's permission-create finding is one case of a wider problem, though. Every put, delete and destroy in all seven file stores (group, nameserver, permission, session, user, zone, zone_record) still does this:

const rows = await this._load()   // read outside the lock
// …modify rows…
await this._save(rows)            // write the stale list back

save() only waits for earlier writes to that file to finish; it doesn't re-read the file. Any create that commits between the _load() and the _save() gets overwritten. See lib/zone/store/file.js:142-170, for example.

To reproduce, run this from the package root against 63c57d0:

import fs from 'node:fs'
process.env.NICTOOL_DATA_STORE = 'json'
process.env.NICTOOL_DATA_STORE_PATH = fs.mkdtempSync('/tmp/store-')
const { default: Zone } = await import('./lib/zone/store/file.js')
const z = new Zone()
const seed = await z.create({ gid: 1, zone: 'seed.example' })
let lost = 0
for (let i = 0; i < 200; i++) {
  const [id] = await Promise.all([
    z.create({ gid: 1, zone: `n${i}.example` }),
    z.put({ id: seed, description: `d${i}` }),
  ])
  if ((await z.get({ id })).length === 0) lost++
}
console.log(`create lost to a concurrent put: ${lost}/200`)

It prints 200/200.

Suggested fix: move each load-modify-save into this.file.mutate(key, (rows) => …), the same pattern create uses. Filter-style destroy needs to splice in place, because mutate writes back the array it passed in. Session put has two save paths. Once nothing calls save() outside the lock, save() and the _save* helpers can go. A test like the permission one, with a put injected after the read, would cover the change.

[aberoham]

Done in 4f76654. Your repro now prints 0/200.

[Copilot]

🟡 Changes recommended

Concurrent internal permission creates can still persist duplicate explicit IDs despite serialized writes.

2 open findings

• Race allows duplicate explicit IDs across concurrent creates · New
• save() only serializes the write, not a caller's preceding read/check/update sequence.…

2 resolved since last review

• Preserve high-water mark when upgrading files without last_id
• Allocate IDs for target-less permissions in standalone fallback

🧠 Review effort: Balanced

[msimerson]

Open items, most serious first.

Fix before merge

1. A permission with no user or group returns 500 (lib/permission/store/file.js:133). Validate 1.0.1 accepts { name: 'x' } with no user or group. The json and toml stores throw a plain Error for it, which Hapi turns into a 500, while MySQL returns 201. Either require a target in the POST schema, or throw a 400 from the store. Then add a route test on json and toml.
2. Permission lookup order differs between reading and changing (lib/permission/store/file.js:213). get({id}) searches users, then standalone, and skips groups. _change() searches users, then groups, then standalone. So a put, delete or destroy on a permission id can change group N's inline permission when GET returned standalone permission N. This was already on main, but now that user and group ids both start at 1, matching ids are common.
3. _write isn't atomic (lib/store/file.js:95). fs.writeFile empties the file before writing it. load() and dns-nameserver's FileSource read without the lock, so they can get an empty or partial file and fail to parse. FileSource's comment says the API writes a temp file and renames it, which isn't true. Fix: write a temp file in the same directory, then rename().

Lower risk

4. One bad row id blocks every create for that file (lib/store/file.js:46). nextId throws on any row id that isn't an unsigned integer, so a hand-edited id: null makes every create for that entity return 500. mutate()'s scan for files without last_id skips the same rows silently, so the two disagree.
5. Explicit-id permission create checks, then acts (lib/permission/store/file.js:72). The duplicate check reads three files outside any lock, and the insert runs later in its own mutate(). Only internal callers can pass an id (HTTP refuses it), so the realistic way to hit this is two concurrent fixture or import runs.
6. Race tests cover only the group store and the login hash upgrade. Nothing tests concurrent writes for zone, nameserver, zone_record, session or user.
7. mutate() rewrites unchanged files. It writes whenever the callback returns anything but false. Session create finding an existing session, and ifExists: 'return' conflicts, both rewrite the file with nothing changed.

Cleanup

8. Id allocation is copied five times (conflict check, nextId, then last_id = max), across the group, user, zone, nameserver and zone_record create callbacks. mutate() has a sixth version with different validity rules, which is why item 4 exists. Move it into one FileStore helper.
9. Stale doc comment (lib/store/file.js:55). The FileStore comment says each file holds an array of rows under one key, but files now also carry last_id.
10. WHAT comments to delete:
    • lib/session/store/file.js:98 was rewritten in place instead of deleted.
    • lib/user/store/file.js:181 keeps the two "Switch to…" comments.
    • test/seed.js:1 restates the helper's body.

[aberoham]

Answered in ee6b118, plus 33c1483 for item 3.

1. A permission with no user or group answers 400 on every store, with a route test. Over HTTP, mysql answered 500 too, from the response check.
2. get, put, delete and destroy now look in the same order: users, standalone, then groups. Ids still collide across the three files, so a group and a standalone permission with one id can't both be reached by id.
3. A write goes to a temp file renamed over the store file. This is store: write file stores atomically #81, which I'll close.
   4 and 8. One claimId() allocates for every store, and nextId skips a bad row id.
4. File-store permission creates run one at a time.
5. Race tests cover put, delete and destroy on every file store.
6. mutate() hands callbacks keep(), so those two paths leave the file alone.
   9 and 10. Done.

[msimerson]

Regression from the item 2 fix: get({id}) now falls back to group permissions, so GET and DELETE /permission/{id} can reach a group's permission that has the same id as a user. The comment Abraham deleted warned about exactly this. I confirmed it with a probe on json:

1. Group 1 and user 1 are created, and user 1 gets its own permission (inherit_group_permissions: false).
2. User 1's permission is destroyed, which also happens when the user is switched back to inheriting.
3. get({ id: 1 }) now returns "Group g.example perms". At 4f76654 it returned nothing, so the route answered 404.
4. DELETE /permission/1 now soft-deletes the group's permissions. A second destroy wiped them in the probe.

Abraham's reply admits that ids collide across the user, group and standalone files. What's new is that HTTP can now reach the collision. Switching a user back to inheriting is a normal UI action, so this is likely to happen.

Possible fixes:

• Leave groups out of get({id}) and _change(). Group permissions stay reachable through { gid }.
• Or give the file stores one permission id space shared across all three files.

[aberoham]

You're right, fixed in fe5b78a, with your repro as a test.