Date: 2026-09-27. Three reviewers (concurrency, security, regressions) ran the code and tried to break it; every finding was re-checked by two skeptics told to refute it. A finding counts as confirmed when both skeptics agree it is real.
Result: 14 findings, 13 confirmed and 1 split; none breaks M1 as used today (root runs sc by hand, one command at a time). Most matter once the M2 watcher runs next to sc, so they are fixed before M2 (round 2, R2-1 to R2-5) or covered by steps in the M2 plan.
Raw results (with full evidence) are kept on the VM in ~/smartconfig-work/m2plan-results.json.
Checked and fine: 10 items, for example:
| # | Finding | Reviewer severity | Skeptics | What we do |
|---|---|---|---|---|
| 1 | A failed COMMIT leaves the write transaction open on a pooled connection; the deferred Rollback does nothing (internal/store/store.go:216) |
medium | confirmed (medium, medium) | Fix now (R2-1). Write transactions run on a dedicated connection with explicit BEGIN/COMMIT/ROLLBACK; COMMIT is retried on SQLITE_BUSY once the file is in place; a connection is never returned to the pool with a transaction open. |
| 2 | Restore now holds the DB write lock across unbounded I/O on the target file’s filesystem (internal/store/store.go:364-373) |
medium | confirmed (low, low) | Fix now (R2-2). The temp file is written and fsynced before the lock is taken; only the rename and the directory fsync happen under the lock. |
| 3 | The pre-restore snapshot is read outside the restore’s transaction, so a concurrent edit is lost and a concurrent restore records a stale pre-restore row (internal/store/store.go:345) |
low | confirmed (low, low) | Fix now (R2-3). The pre-restore read and row move inside the restore transaction, so a concurrent restore records the true previous state. A concurrent editor (who never takes the lock) can still lose an edit made in the few milliseconds between the read and the rename; documented as a known limit. |
| 4 | The ordering only protects observers that take the write lock; a read-only observer still sees an unexplained change (internal/store/store.go:358-362) |
low | confirmed (low, medium) | Contract, covered by the M2 plan. Observers must take the write lock before deciding a change is unexplained; the plan’s watcher re-checks the file stamp under the write lock (plan section 5.5). |
Checked and fine: 11 items, for example:
| # | Finding | Reviewer severity | Skeptics | What we do | |
|---|---|---|---|---|---|
| 1 | Symlinked parent directories still redirect ReadWithMeta and WriteAtomic (O_NOFOLLOW covers only the last component) (internal/fsutil/open_linux.go:15) |
high | confirmed (medium, low) | Covered by M2 plan step 6 (reads through a parent directory handle with openat) and plan question 4 (refuse restores into non-root or symlinked directories). Not reachable in M1 as used: root runs sc on root-owned paths. | |
| 2 | WriteAtomic’s directory fsync open blocks on a FIFO, and since ec15ece it holds the database write lock while blocked (internal/fsutil/fsutil.go:119) |
medium | confirmed (low, medium) | Fix now (R2-2). The directory is opened with O_DIRECTORY | O_NONBLOCK, so a FIFO in its place fails at once instead of blocking under the lock. (Corrected after the round-2 review: it deliberately follows a symlinked directory, because the fsync must reach the real directory; an earlier version of this line said O_NOFOLLOW.) |
| 3 | O_NONBLOCK makes open of a regular file under a write lease fail at once with an unclear error; restore aborts (internal/fsutil/open_linux.go:15) |
low | confirmed (low, low) | Deferred to M2 step 6. Only files under a kernel write lease (Samba/NFS servers) are affected; the reopen through a parent directory handle in step 6 can check the type first and drop O_NONBLOCK. | |
| 4 | ELOOP mapping reports a looping or too-deep directory prefix as ‘is a symlink, refusing’ and drops the real error (internal/fsutil/open_linux.go:16-17) |
low | confirmed (low, low) | Fix now (R2-5). ELOOP is reported as “is a symlink” only when the last component really is one; otherwise the real error is kept. |
Checked and fine: 16 items, for example:
| # | Finding | Reviewer severity | Skeptics | What we do |
|---|---|---|---|---|
| 1 | The restore lock does not protect Snapshot: its ‘unchanged’ check reads the latest row before taking the write lock, so a snapshot taken in the restore window records the restored content as a new row (internal/store/store.go:141) |
medium | confirmed (low, low) | Fix now (R2-4). Snapshot’s “unchanged” check moves inside the write transaction, so a snapshot taken during a restore waits for it and sees the restore row. |
| 2 | A COMMIT that fails with SQLITE_BUSY leaves the SQLite transaction open on a pooled connection, still holding the write lock (internal/store/store.go:216) |
medium | confirmed (low, low) | Same as concurrency 1 (R2-1). |
| 3 | A failed restore still commits a ‘pre-restore’ row every time it is attempted; the new ‘records nothing’ test covers only the one case where no pre-restore row is taken (internal/store/store.go:345) |
low | split (not-a-problem, low) | Fixed by R2-3 (split vote): with the pre-restore row inside the restore transaction, a failed restore records nothing at all. |
| 4 | Restore now returns database errors without context, e.g. ‘sc: begin: database is locked’, which does not say it was a restore or that the file is unchanged (internal/store/store.go:378) |
low | confirmed (low, low) | Fix now (R2-3). Restore errors say it was the restore that failed and whether the file was changed. |
| 5 | Restore now holds the SQLite write lock during the whole temp-file write and two fsyncs (about 3x longer for an 8 MB file) (internal/store/store.go:364) |
low | confirmed (low, low) | Fix now (R2-2), as concurrency 2. |
| 6 | O_NONBLOCK makes snapshot fail at once with EAGAIN on a file another process holds a write lease on; before, it waited and succeeded (internal/fsutil/open_linux.go:15) |
low | confirmed (low, low) | Deferred to M2 step 6, as security 3. |
| Id | Change |
|---|---|
| R2-1 | store: write transactions on a dedicated connection; COMMIT retried on SQLITE_BUSY; never return a connection with an open transaction |
| R2-2 | fsutil/store: write and fsync the temp file before taking the lock; only rename and directory fsync under the lock; directory opened with O_DIRECTORY|O_NONBLOCK (follows a symlinked directory on purpose) |
| R2-3 | store: restore in one transaction (pre-restore read and row, restore row, rename, commit); errors say whether the file changed |
| R2-4 | store: snapshot’s unchanged check inside the write transaction |
| R2-5 | fsutil: precise ELOOP message |