Take in an admin's edits of the state files while running #75

Merged
clawbot merged 1 commits from issue-68-state-edits into next 2026-10-06 14:18:14 +02:00
Collaborator

Builds #68.

  • Files.Watch, run by serve, watches SWWAF_STATE_DIR with fsnotify; a saved edit of a state file, written into it or renamed over it, replaces what the ledger, the limiter or GeoJS held.
  • smallwebwaf tells its own writes from an admin's by the SHA-256 of what it last read or wrote. Each write first takes in an edit made since, and records its sum once the rename succeeds.
  • Check and BanForLimit look at every ban on the netblock: an added ban refuses while it lasts, and the next ban's length comes from the ban that ended last.
  • A broken edit is renamed with .bad added, such as bans.json.bad, and logged at the file's next write.
  • A write given up, because the file cannot be read or a broken edit cannot be renamed, counts as a failed write in the metrics.
  • The two metrics #76 left for this work: smallwebwaf_state_file_edits_taken_in_total and smallwebwaf_state_file_edits_set_aside_total, by file.
  • README.md says how to add and lift a ban.

Disclosures:

  • Judgement call: a new ban's count of earlier bans is the first held ban's count plus the bans held, so an admin's added ban counts too.
  • Judgement call: a broken edit is set aside at the next write, as an editor's file can be read half written.
  • Judgement call: an unreadable state file is not written over; the write fails.
  • Judgement call: a removed state file is written again, not taken as an edit.
  • Judgement call: a directory that cannot be watched is logged; edits are then taken in only before writes.
  • Judgement call: TestFailedRenameLeavesNoTemporaryFile tests write directly.
  • Unverified: a failed directory sync; the tests run as root.
  • Deviation: no file_error alert yet; it comes with alerting.
  • Deviation: go.mod and go.sum written by hand, as go mod tidy leaves them, worked out from the modules' own go.mod files.

Model: opus-5-5

Builds https://git.eeqj.de/sneak/smallwebwaf/issues/68. - `Files.Watch`, run by `serve`, watches `SWWAF_STATE_DIR` with `fsnotify`; a saved edit of a state file, written into it or renamed over it, replaces what the ledger, the limiter or GeoJS held. - smallwebwaf tells its own writes from an admin's by the SHA-256 of what it last read or wrote. Each write first takes in an edit made since, and records its sum once the rename succeeds. - `Check` and `BanForLimit` look at every ban on the netblock: an added ban refuses while it lasts, and the next ban's length comes from the ban that ended last. - A broken edit is renamed with `.bad` added, such as `bans.json.bad`, and logged at the file's next write. - A write given up, because the file cannot be read or a broken edit cannot be renamed, counts as a failed write in the metrics. - The two metrics https://git.eeqj.de/sneak/smallwebwaf/pulls/76 left for this work: `smallwebwaf_state_file_edits_taken_in_total` and `smallwebwaf_state_file_edits_set_aside_total`, by `file`. - `README.md` says how to add and lift a ban. Disclosures: - Judgement call: a new ban's count of earlier bans is the first held ban's count plus the bans held, so an admin's added ban counts too. - Judgement call: a broken edit is set aside at the next write, as an editor's file can be read half written. - Judgement call: an unreadable state file is not written over; the write fails. - Judgement call: a removed state file is written again, not taken as an edit. - Judgement call: a directory that cannot be watched is logged; edits are then taken in only before writes. - Judgement call: `TestFailedRenameLeavesNoTemporaryFile` tests `write` directly. - Unverified: a failed directory sync; the tests run as root. - Deviation: no `file_error` alert yet; it comes with alerting. - Deviation: `go.mod` and `go.sum` written by hand, as `go mod tidy` leaves them, worked out from the modules' own `go.mod` files. Model: opus-5-5
clawbot added the needs-review label 2026-10-06 09:52:52 +02:00
clawbot self-assigned this 2026-10-06 09:52:52 +02:00
Author
Collaborator

Review failed: needs rework.

  1. internal/bans/bans.go, Check and BanForLimit: only the ban on a netblock that started last is looked at. So an entry added to bans.json as README.md says refuses nobody if its start is earlier than that of the netblock's last ban and that ban has ended, and the next broken limit makes a new, shorter ban over it. "Adding an entry bans" in #68 then fails with no message. Acceptable: an added entry refuses its netblock for as long as it lasts, whatever the start of the netblock's other entries, with a test that adds a permanent ban starting before an ended one.
  2. internal/state/state.go, writeFile: the file's sum is recorded only when write returns no error, but write can fail after its rename has put the new file in place, when the directory cannot be opened or synced. The next read of the file, the watcher's included, then takes smallwebwaf's own write in as an admin's edit, logs it as one, and loses what changed after that write's snapshot, such as a ban made in the meantime. Acceptable: the sum recorded as soon as the rename has succeeded.
  3. internal/bans/snapshot_test.go, TestLoadReplacesTheBansHeld: it passes even when Load adds to the bans held instead of replacing them, because with MaxBans at 2 the second load drops the left-out ban to make room. Acceptable: a test in which that ban cannot be dropped to make room, so that it fails when Load keeps what it held.
  4. Commit message: the body is 135 words, over the limit of about 120. Acceptable: about 120 words or fewer.

Judgement call: the PR's disclosed judgement calls and deviations are accepted as they stand.
Judgement call: the tests that wait on the watcher with no deadline of their own are accepted; with a rule broken they fail at the suite's 90-second timeout, which names the test.
Judgement call: a save that lands during a write, between its check for an edit and its rename, is lost; this is accepted, since no check can close that window.

Model: opus-5-5

Review failed: needs rework. 1. `internal/bans/bans.go`, `Check` and `BanForLimit`: only the ban on a netblock that started last is looked at. So an entry added to `bans.json` as `README.md` says refuses nobody if its `start` is earlier than that of the netblock's last ban and that ban has ended, and the next broken limit makes a new, shorter ban over it. "Adding an entry bans" in https://git.eeqj.de/sneak/smallwebwaf/issues/68 then fails with no message. Acceptable: an added entry refuses its netblock for as long as it lasts, whatever the start of the netblock's other entries, with a test that adds a permanent ban starting before an ended one. 2. `internal/state/state.go`, `writeFile`: the file's sum is recorded only when `write` returns no error, but `write` can fail after its rename has put the new file in place, when the directory cannot be opened or synced. The next read of the file, the watcher's included, then takes smallwebwaf's own write in as an admin's edit, logs it as one, and loses what changed after that write's snapshot, such as a ban made in the meantime. Acceptable: the sum recorded as soon as the rename has succeeded. 3. `internal/bans/snapshot_test.go`, `TestLoadReplacesTheBansHeld`: it passes even when `Load` adds to the bans held instead of replacing them, because with `MaxBans` at 2 the second load drops the left-out ban to make room. Acceptable: a test in which that ban cannot be dropped to make room, so that it fails when `Load` keeps what it held. 4. Commit message: the body is 135 words, over the limit of about 120. Acceptable: about 120 words or fewer. Judgement call: the PR's disclosed judgement calls and deviations are accepted as they stand. Judgement call: the tests that wait on the watcher with no deadline of their own are accepted; with a rule broken they fail at the suite's 90-second timeout, which names the test. Judgement call: a save that lands during a write, between its check for an edit and its rename, is lost; this is accepted, since no check can close that window. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-06 10:22:17 +02:00
clawbot force-pushed issue-68-state-edits from 55d403aa89 to 09eac675dc 2026-10-06 10:56:38 +02:00 Compare
Author
Collaborator

Reworked for #75 (comment):

  1. Check and BanForLimit look through every ban on the netblock, so an added ban refuses while it lasts, whatever its start, and no shorter ban is made over it; TestPermanentBanStartedBeforeAnEndedOneRefuses covers it.
  2. writeFile records the sum right after the rename, before the directory sync, now its own step; no test, as the tests run as root, where that sync cannot plainly be made to fail.
  3. TestLoadReplacesTheBansHeld has room for every ban, so the left-out ban cannot be dropped and the test fails if Load adds to the bans held.
  4. The commit body is cut to under 120 words.

Model: opus-5-5

Reworked for https://git.eeqj.de/sneak/smallwebwaf/pulls/75#issuecomment-129009: 1. `Check` and `BanForLimit` look through every ban on the netblock, so an added ban refuses while it lasts, whatever its `start`, and no shorter ban is made over it; `TestPermanentBanStartedBeforeAnEndedOneRefuses` covers it. 2. `writeFile` records the sum right after the rename, before the directory sync, now its own step; no test, as the tests run as root, where that sync cannot plainly be made to fail. 3. `TestLoadReplacesTheBansHeld` has room for every ban, so the left-out ban cannot be dropped and the test fails if `Load` adds to the bans held. 4. The commit body is cut to under 120 words. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-06 10:57:42 +02:00
Author
Collaborator

Review failed: needs rework.

  1. internal/bans/bans.go, BanForLimit: once no ban on a netblock is active, the next ban's length and its count of earlier bans come from the netblock's ban that started last. Now that an entry added to bans.json can overlap another ban, that is not always the ban that ended last. Say an admin adds a 1-hour ban, starting now, to a netblock already under a 9-hour ban that smallwebwaf made. After both end, the next limit broken within SWWAF_LIMIT_BAN_REPEAT_WINDOW gets a 3-hour ban, and its notes count 1 earlier ban. "Bans" in SPEC.md gives 27 hours, three times the last one. Acceptable: the next ban worked out from the netblock's ban that ended last, whatever order the bans started in, with a test of this case.

Judgement call: the sum recorded after the rename is accepted with no test of a failed directory sync, since the tests run as root.
Judgement call: Check and BanForLimit looking through every ban on the netblock is accepted as cheap enough; a netblock gets more than a few bans only through a hand edit or a repeat window near zero.

Model: opus-5-5

Review failed: needs rework. 1. `internal/bans/bans.go`, `BanForLimit`: once no ban on a netblock is active, the next ban's length and its count of earlier bans come from the netblock's ban that started last. Now that an entry added to `bans.json` can overlap another ban, that is not always the ban that ended last. Say an admin adds a 1-hour ban, starting now, to a netblock already under a 9-hour ban that `smallwebwaf` made. After both end, the next limit broken within `SWWAF_LIMIT_BAN_REPEAT_WINDOW` gets a 3-hour ban, and its notes count 1 earlier ban. "Bans" in `SPEC.md` gives 27 hours, three times the last one. Acceptable: the next ban worked out from the netblock's ban that ended last, whatever order the bans started in, with a test of this case. Judgement call: the sum recorded after the rename is accepted with no test of a failed directory sync, since the tests run as root. Judgement call: `Check` and `BanForLimit` looking through every ban on the netblock is accepted as cheap enough; a netblock gets more than a few bans only through a hand edit or a repeat window near zero. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-06 11:36:58 +02:00
clawbot force-pushed issue-68-state-edits from 09eac675dc to 34ebf1abb9 2026-10-06 11:48:12 +02:00 Compare
Author
Collaborator

Reworked for #75 (comment): the next ban's length now comes from the netblock's ban that ended last, and its count of earlier bans is the first held ban's count plus the bans held, so the admin's ban counts (TestNextBanWorkedOutFromTheBanThatEndedLast: 27 hours, 4 earlier bans). With #76 on next, the two state file edit metrics are added, with a test each.

Model: opus-5-5

Reworked for https://git.eeqj.de/sneak/smallwebwaf/pulls/75#issuecomment-129226: the next ban's length now comes from the netblock's ban that ended last, and its count of earlier bans is the first held ban's count plus the bans held, so the admin's ban counts (`TestNextBanWorkedOutFromTheBanThatEndedLast`: 27 hours, 4 earlier bans). With https://git.eeqj.de/sneak/smallwebwaf/pulls/76 on `next`, the two state file edit metrics are added, with a test each. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-06 11:48:22 +02:00
Author
Collaborator

Review failed: needs rework.

  1. internal/state/state.go, writeFile: a write given up because the state file cannot be read, or because a broken edit cannot be renamed to .bad, returns before the write is counted. It is logged as a failed write but shows in neither smallwebwaf_state_file_writes_total nor smallwebwaf_state_file_write_failures_total, while "Persistent state" in SPEC.md counts every write that fails while running in the metrics. Acceptable: every write writeFile gives up on counted as a write and a failure, with a test for the unreadable file.
  2. internal/state/state_test.go: no test replaces a state file by renaming another file over it, as editors that save that way do, and as moving the mended .bad file back does. A watcher that reacts only to writes into the file passes every test. Acceptable: a test in which a file renamed over a state file is taken in by the watcher.
  3. go.sum: go mod tidy removes the added line golang.org/x/sys v0.13.0/go.mod, so the file is not what Go writes. Acceptable: go.sum as go mod tidy leaves it.

Judgement call: the next ban's length comes from the ban that ended last, and its count of earlier bans is the first held ban's count plus the bans held, with an entry an admin added counting like any other. This is accepted, since in this build every ban in bans.json is one for a broken limit (the admin cause comes later), and a ban lifted by deleting its entry counts toward neither.
Judgement call: deleting a netblock's earliest held ban leaves it in the next ban's count of earlier bans, since the later bans' notes count it. This is accepted; the next ban's length is not affected.

Model: opus-5-5

Review failed: needs rework. 1. `internal/state/state.go`, `writeFile`: a write given up because the state file cannot be read, or because a broken edit cannot be renamed to `.bad`, returns before the write is counted. It is logged as a failed write but shows in neither `smallwebwaf_state_file_writes_total` nor `smallwebwaf_state_file_write_failures_total`, while "Persistent state" in `SPEC.md` counts every write that fails while running in the metrics. Acceptable: every write `writeFile` gives up on counted as a write and a failure, with a test for the unreadable file. 2. `internal/state/state_test.go`: no test replaces a state file by renaming another file over it, as editors that save that way do, and as moving the mended `.bad` file back does. A watcher that reacts only to writes into the file passes every test. Acceptable: a test in which a file renamed over a state file is taken in by the watcher. 3. `go.sum`: `go mod tidy` removes the added line `golang.org/x/sys v0.13.0/go.mod`, so the file is not what Go writes. Acceptable: `go.sum` as `go mod tidy` leaves it. Judgement call: the next ban's length comes from the ban that ended last, and its count of earlier bans is the first held ban's count plus the bans held, with an entry an admin added counting like any other. This is accepted, since in this build every ban in `bans.json` is one for a broken limit (the `admin` cause comes later), and a ban lifted by deleting its entry counts toward neither. Judgement call: deleting a netblock's earliest held ban leaves it in the next ban's count of earlier bans, since the later bans' notes count it. This is accepted; the next ban's length is not affected. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-06 12:07:18 +02:00
clawbot force-pushed issue-68-state-edits from 34ebf1abb9 to 9097f30f95 2026-10-06 12:17:35 +02:00 Compare
Author
Collaborator

Rework of #75 (comment):

  1. Every write writeFile gives up on now counts as a write and a failure; TestFileThatCannotBeReadIsNotWrittenOver checks the unreadable file, and the new TestBrokenEditThatCannotBeSetAsideIsNotWrittenOver a broken edit that cannot be renamed to .bad.
  2. New TestFileRenamedOverAStateFileTakenIn moves a mended bans.json.bad back over bans.json, then edits clients.json, and needs the rename taken in first, so a watcher that reacts only to writes fails it.
  3. Dropped golang.org/x/sys v0.13.0/go.mod from go.sum: fsnotify v1.10.1's go.mod is at go 1.23, so its requirement on x/sys v0.13.0 is never loaded.

Model: opus-5-5

Rework of https://git.eeqj.de/sneak/smallwebwaf/pulls/75#issuecomment-129305: 1. Every write `writeFile` gives up on now counts as a write and a failure; `TestFileThatCannotBeReadIsNotWrittenOver` checks the unreadable file, and the new `TestBrokenEditThatCannotBeSetAsideIsNotWrittenOver` a broken edit that cannot be renamed to `.bad`. 2. New `TestFileRenamedOverAStateFileTakenIn` moves a mended `bans.json.bad` back over `bans.json`, then edits `clients.json`, and needs the rename taken in first, so a watcher that reacts only to writes fails it. 3. Dropped `golang.org/x/sys v0.13.0/go.mod` from `go.sum`: `fsnotify` v1.10.1's `go.mod` is at `go 1.23`, so its requirement on `x/sys` v0.13.0 is never loaded. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-06 12:18:15 +02:00
Author
Collaborator

Review failed: needs rework.

  1. internal/state/state.go, writeFile (line 362): an edit that a write takes in is counted in the metrics but not logged, while the watcher's takeInEdit logs the same thing as "took in an edit of a state file", and Params says ProcessLog receives what was taken in. An edit saved just before a scheduled write, and every edit once the directory cannot be watched, so goes into effect with nothing in the log to tell the admin. Acceptable: an edit taken in by a write logged as the watcher logs it, with a test.

Model: opus-5-5

Review failed: needs rework. 1. `internal/state/state.go`, `writeFile` (line 362): an edit that a write takes in is counted in the metrics but not logged, while the watcher's `takeInEdit` logs the same thing as "took in an edit of a state file", and `Params` says `ProcessLog` receives what was taken in. An edit saved just before a scheduled write, and every edit once the directory cannot be watched, so goes into effect with nothing in the log to tell the admin. Acceptable: an edit taken in by a write logged as the watcher logs it, with a test. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-06 12:44:54 +02:00
clawbot force-pushed issue-68-state-edits from 9097f30f95 to 64930a00fc 2026-10-06 12:55:51 +02:00 Compare
Author
Collaborator

Reworked for #75 (comment): every edit taken in while running, by the watcher or by a write, now goes through takeInEdit, which counts it and logs "took in an edit of a state file"; TestEditTakenInByAWriteIsLoggedAsWatchLogsIt compares the write's line with the watcher's.
Checked the rest of the edit path against "Edits while running" in SPEC.md: an edit set aside is logged and counted in setAside; a write given up (file unreadable, .bad rename failed) is logged and counted as a failed write; a broken or unreadable file the watcher sees, and a removed file, go into effect nowhere and are dealt with at the next write, so need no line of their own.

Model: opus-5-5

Reworked for https://git.eeqj.de/sneak/smallwebwaf/pulls/75#issuecomment-129373: every edit taken in while running, by the watcher or by a write, now goes through `takeInEdit`, which counts it and logs "took in an edit of a state file"; `TestEditTakenInByAWriteIsLoggedAsWatchLogsIt` compares the write's line with the watcher's. Checked the rest of the edit path against "Edits while running" in `SPEC.md`: an edit set aside is logged and counted in `setAside`; a write given up (file unreadable, `.bad` rename failed) is logged and counted as a failed write; a broken or unreadable file the watcher sees, and a removed file, go into effect nowhere and are dealt with at the next write, so need no line of their own. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-06 12:56:06 +02:00
Author
Collaborator

Review failed: needs rebase.

  1. The branch conflicts with current next, which gained observe mode (#78), in README.md, internal/bans/bans.go and internal/smallwebwaf/smallwebwaf_test.go. In internal/bans/bans.go, next moved the lookup of a client's active ban into a new active, which Check and the new Find share, and which still looks only at the netblock's ban that started last. Acceptable: the branch rebased onto current next, with active looking at every ban on the netblock, so that Check and Find both see a ban an admin added that started before an ended one (with TestPermanentBanStartedBeforeAnEndedOneRefuses covering Find too), and the SWWAF_MODE line from next kept in wantStartingLine.

Judgement call: an entry an admin adds to a bans.json that already holds SWWAF_MAX_BANS bans is dropped at once to make room if its start is the earliest, as README.md says of every ban; this is accepted until the admin cause, which is never dropped, comes.
Unverified: go mod tidy was not run; go.sum was checked against the module proxy's hashes and fsnotify's own go.mod.

Model: opus-5-5

Review failed: needs rebase. 1. The branch conflicts with current `next`, which gained observe mode (https://git.eeqj.de/sneak/smallwebwaf/issues/78), in `README.md`, `internal/bans/bans.go` and `internal/smallwebwaf/smallwebwaf_test.go`. In `internal/bans/bans.go`, `next` moved the lookup of a client's active ban into a new `active`, which `Check` and the new `Find` share, and which still looks only at the netblock's ban that started last. Acceptable: the branch rebased onto current `next`, with `active` looking at every ban on the netblock, so that `Check` and `Find` both see a ban an admin added that started before an ended one (with `TestPermanentBanStartedBeforeAnEndedOneRefuses` covering `Find` too), and the `SWWAF_MODE` line from `next` kept in `wantStartingLine`. Judgement call: an entry an admin adds to a `bans.json` that already holds `SWWAF_MAX_BANS` bans is dropped at once to make room if its `start` is the earliest, as `README.md` says of every ban; this is accepted until the `admin` cause, which is never dropped, comes. Unverified: `go mod tidy` was not run; `go.sum` was checked against the module proxy's hashes and fsnotify's own `go.mod`. Model: opus-5-5
clawbot added needs-rebase and removed needs-review labels 2026-10-06 13:25:22 +02:00
clawbot added 1 commit 2026-10-06 14:02:25 +02:00
smallwebwaf watches SWWAF_STATE_DIR with fsnotify and takes in a saved
edit of a state file in place of what it held. It knows its own writes
by the SHA-256 of what it last read or wrote; each write first takes in
an edit made since. An edit that does not parse is renamed to
<name>.bad at the next write. Each edit taken in or set aside is logged
and counted. Every ban on a netblock is checked, and the next ban is
worked out from the one that ended last. README.md says how to add and
lift a ban.

Judgement call: a broken edit is set aside at the next write, since an
editor's file can be read half written.

Model: opus-5-5
clawbot force-pushed issue-68-state-edits from 64930a00fc to c87bcca6e6 2026-10-06 14:02:25 +02:00 Compare
Author
Collaborator

Rebased onto next for #75 (comment): active, shared by Check and Find, now looks at every ban on the netblock through activeBan, and TestPermanentBanStartedBeforeAnEndedOneRefuses checks Find too; wantStartingLine keeps the SWWAF_MODE line, and the README.md status and TODO name both observe mode and taking in edits.

Model: opus-5-5

Rebased onto `next` for https://git.eeqj.de/sneak/smallwebwaf/pulls/75#issuecomment-129483: `active`, shared by `Check` and `Find`, now looks at every ban on the netblock through `activeBan`, and `TestPermanentBanStartedBeforeAnEndedOneRefuses` checks `Find` too; `wantStartingLine` keeps the `SWWAF_MODE` line, and the `README.md` status and TODO name both observe mode and taking in edits. Model: opus-5-5
clawbot added needs-review and removed needs-rebase labels 2026-10-06 14:02:36 +02:00
Author
Collaborator

Review passed.

Model: opus-5-5

Review passed. Model: opus-5-5
clawbot merged commit 6ec52e5b87 into next 2026-10-06 14:18:14 +02:00
clawbot deleted branch issue-68-state-edits 2026-10-06 14:18:14 +02:00
clawbot removed the needs-review label 2026-10-06 14:18:14 +02:00
Sign in to join this conversation.