Date: 2026-09-27. Same method as round 1: three reviewers (transactions, files, regressions) ran the code and tried to break it; two skeptics re-checked every finding. 15 findings, all confirmed; many are the same issue seen through different lenses, so they are grouped below.
Raw results with full evidence: ~/smartconfig-work/review-r2-results.json on the VM.
| Id | Issue | Seen by | Severity | What we do |
|---|---|---|---|---|
| U1 | A restore whose COMMIT fails (or that is killed) after the rename loses the pre-restore row, so the overwritten content is only an unreferenced blob. A regression from R2-3: before it, the pre-restore row was committed first. | all three | medium | R3-1: commit the pre-restore row in its own transaction before the rename again; under the restore lock, re-check the file’s stamp (device, inode, size, mtime, ctime) and start over if it changed, so a concurrent restore still cannot make it stale. The error names the saved pre-restore id. |
| U2 | Timestamps are taken before the write lock, so concurrent restores are logged out of commit order; a snapshot can describe content that changed before it committed. | transactions, regressions | medium/low | R3-2: take timestamps under the lock; sc snapshot re-checks the file’s stamp under the lock and re-reads if it changed. |
| U3 | R2-3 moved the pre-restore read, hash and blob write back under the lock (an 8 MB restore held it 0.4-1.1 s instead of 0.02-0.2 s), undoing most of R2-2. | all three | low | R3-1: the read, hash and blob write happen before the lock; under it only a stat. |
| U4 | COMMIT is retried for up to about 30 s for every write, holding SQLite’s PENDING lock, so meanwhile even sc log fails; a snapshot has nothing on disk to protect. |
transactions, regressions | low | R3-3: retry COMMIT only when the caller has already changed a file on disk (a restore after its rename). |
| U5 | writeTx is not panic-safe: a recovered panic would pool a connection with its transaction still open. |
transactions | low | R3-3: roll back (or discard the connection) on panic, then re-panic. |
| U6 | Interrupting sc restore (Ctrl-C, SIGTERM) while it waits for the lock leaves the prepared temp file, a full copy, in the config directory. |
files, regressions | low | R3-4: on SIGINT/SIGTERM, sc removes its pending temp files and exits with one line. (kill -9 cannot be caught; M2’s watcher cleans stale .*.sc-tmp-* at start.) |
| U7 | Errors from the prepare step lack “(file not changed)”. | files | low | Fixed in R3-1 (e7420bb; this row first said R3-5). |
| U8 | The round-1 review doc says syncDir uses O_NOFOLLOW; the code rightly does not, and no test guards that. |
files | low | R3-5: correct the doc; test syncDir through a symlinked directory. |