From: Willy Tarreau Date: Mon, 6 Jul 2026 21:13:10 +0000 (+0200) Subject: DEV: patchbot: update: support replacing a whole note blob (setnotes) X-Git-Url: http://git.kaiwu.me/web//%22%22?a=commitdiff_plain;h=bff33b4e32cab654dca225210e8890c2101c56dc;p=haproxy.git DEV: patchbot: update: support replacing a whole note blob (setnotes) Notes are append-only on the wire, which makes concurrent edits conflict-free but leaves no way to revise or clean up a note from the page: fixing a note requires a hand-edit of the storage file. This adds the replacement directive that the design had reserved: setnotes Unlike the other directives, a replacement must carry a token of the base it was computed from, or it could silently destroy a concurrent update (one reviewer's append landing between another's read and replace). The token is the SDBM hash (8 hex chars) of the note blob the client based its edit on: the server only applies the replacement if it still matches the stored blob (empty-string hash for a commit without notes). On mismatch the directive is dropped, the line is left exactly as found, and a "conflict " line is emitted in the response before the resulting lines so that the client can point the user at what needs manual reconciliation; other directives from the same POST are still applied, and nothing is written nor committed when everything conflicted. SDBM is trivially computed on both sides (a concurrency token, not a security feature, and JS crypto is unavailable on plain http anyway), and its small multiplier keeps the whole hash computation exact in awk's double-precision arithmetic, which is precisely why it was chosen over wider-multiplier hashes. An empty replacement deletes the notes (and the line if no state is left), finally allowing obsolete notes to be removed without editing the file by hand. Replacements are capped to 4000 chars instead of the 500-char append cap, since a coalesced blob may legitimately have grown beyond a single addition. --- diff --git a/dev/patchbot/cgi/update.awk b/dev/patchbot/cgi/update.awk index 9700be027..9f4f7a963 100755 --- a/dev/patchbot/cgi/update.awk +++ b/dev/patchbot/cgi/update.awk @@ -32,6 +32,7 @@ # - POST update.cgi?branch=3.5 with a line-oriented body: # state # notes +# setnotes # "state" overrides the verdict, "revert" (aliases "same", "unchanged") # removes the override so the bot's verdict applies again, "notes" # appends to the commit's notes (capped to 500 chars per push). Broken @@ -39,14 +40,20 @@ # the result is committed to git. Neither directive carries a base/old # value: state is last-write-wins and notes are append-only, which is # what keeps concurrent edits conflict-free. +# "setnotes" replaces the whole note blob (or deletes it when the text +# is empty) and is the exception: is the SDBM hash (8 hex +# chars) of the blob the client based its edit on, and the directive is +# only applied if it still matches the stored blob. Otherwise it is +# dropped and reported as a "conflict " line in the response, so +# that a replacement can never silently destroy a concurrent update. # - GET update.cgi?branch=3.5 returns the current overlay as a JSON array # of {"cid": ..., "state": ..., "notes": ...} objects, with absent # fields omitted and notes fully unescaped (an empty overlay yields # "[]"), directly usable with JSON.parse() on the client. # # Requires GNU awk (PROCINFO, systime); the -b flag in the shebang makes all -# string operations byte-based regardless of the locale, which the escaping -# and the caps depend on. A few points deserve attention: +# string operations byte-based regardless of the locale, which the escaping, +# the caps and the hash depend on. A few points deserve attention: # - external commands (git, mkdir, mv, kill) go through /bin/sh, so # everything interpolated into a command is shell-quoted with q(); # - NUL bytes in inputs are not reliably preserved by awk; they can only @@ -71,6 +78,7 @@ BEGIN { MAX_CID_LEN = 40 # bound on a commit id (full SHA-1) MAX_NOTE_LEN = 500 # cap on a single pushed note addition + MAX_EDIT_LEN = 4000 # cap on a whole-blob replacement (setnotes) MAX_BRANCH_LEN = 15 # bound on the branch name MAX_BODY_LEN = 1048576 # bound on a POST body NOTE_SEP = "; " # separator between coalesced notes @@ -159,6 +167,19 @@ function get_branch( n, i, p, v) return v } +# SDBM hash of (h = c + h * 65599) as 8 hex chars, the concurrency token +# carried by a note blob replacement; must match the page's JS version. The +# small multiplier keeps the 32-bit state exactly representable with awk's +# double-precision numbers (65599 * 2^32 stays well below 2^53). +function sdbm_hex(s, h, i, n) +{ + h = 0 + n = length(s) + for (i = 1; i <= n; i++) + h = (ORD[substr(s, i, 1)] + h * 65599) % 4294967296 + return sprintf("%08x", h) +} + # Symmetric-prefix commit id match: two ids designate the same commit iff # one is a prefix of the other. The caller scans in file order and stops on # the first match; providing enough digits to stay unambiguous is the @@ -364,6 +385,19 @@ function parse_directives(body, nb, n, i, line, cid, rest, v, txt, h) nb++ d_type[nb] = "notes"; d_cid[nb] = cid; d_note[nb] = txt } + else if (match(rest, /^setnotes[ \t]+/)) { + v = substr(rest, RLENGTH + 1) + if (!match(v, /^[0-9a-fA-F]{8}([ \t]|$)/)) + continue + h = tolower(substr(v, 1, 8)) + txt = substr(v, 9) + sub(/^[ \t]+/, "", txt) + # an empty replacement is valid: it deletes the notes + nb++ + d_type[nb] = "setnotes"; d_cid[nb] = cid + d_hash[nb] = h + d_note[nb] = sanitize_note(txt, MAX_EDIT_LEN) + } } return nb } @@ -387,14 +421,18 @@ function load_file(fname, line) # first match wins, scanning the file lines then the new entries; a miss # creates a new entry (except for a revert, which then has nothing to # remove). A line reduced to neither state nor notes is dropped at write -# time. -function apply_directive(di, i, li, ni) +# time. Returns 0 on success, or 1 when a setnotes base hash doesn't match +# the stored blob anymore: the directive is then not applied (a replacement +# must never silently destroy a concurrent update) and the caller reports +# the conflict. +function apply_directive(di, i, li, ni, was_touched, cur) { - li = 0; ni = 0 + li = 0; ni = 0; was_touched = 0 for (i = 1; i <= nb_lines; i++) { if (L_cid[i] == "" || !cid_match(L_cid[i], d_cid[di])) continue li = i + was_touched = L_touched[i] if (!L_touched[i]) { parse_line(L_raw[i]) L_state[i] = P_state @@ -413,10 +451,27 @@ function apply_directive(di, i, li, ni) } } + # The base check happens before any entry creation or modification. + # On conflict the line must be left exactly as found, including not + # marked as modified if this lookup was what materialised it. + if (d_type[di] == "setnotes") { + cur = "" + if (li && L_has[li]) + cur = L_notes[li] + else if (ni && N_has[ni]) + cur = N_notes[ni] + if (sdbm_hex(cur) != d_hash[di]) { + if (li && !was_touched) + L_touched[li] = 0 + return 1 + } + } if (!li && !ni) { if (d_type[di] == "revert") - return # nothing stored for this commit anyway + return 0 # nothing stored for this commit anyway + if (d_type[di] == "setnotes" && d_note[di] == "") + return 0 # deleting non-existing notes nb_new++ N_cid[nb_new] = d_cid[di] N_state[nb_new] = ""; N_notes[nb_new] = ""; N_has[nb_new] = 0 @@ -439,6 +494,17 @@ function apply_directive(di, i, li, ni) N_has[ni] = 1 } } + else if (d_type[di] == "setnotes") { + if (li) { + L_notes[li] = d_note[di] + L_has[li] = (d_note[di] != "") + } + else { + N_notes[ni] = d_note[di] + N_has[ni] = (d_note[di] != "") + } + } + return 0 } # Serialises all writers around the branch files. The lock is a directory @@ -601,7 +667,7 @@ function json_str(s, out, i, n, c) # the POST handler: parse directives, and if any survives, apply them to the # branch file under the lock, atomically replace it and commit it to git. -function handle_post( body, i, fname, attempt, renamed) +function handle_post( body, i, fname, nb_confl, attempt, renamed) { body = read_body() nb_dirs = parse_directives(body) @@ -634,56 +700,69 @@ function handle_post( body, i, fname, attempt, renamed) delete L_raw; delete L_cid; delete L_touched delete L_state; delete L_notes; delete L_has delete N_cid; delete N_state; delete N_notes; delete N_has + delete CONFL load_file(fname) - nb_new = 0 - for (i = 1; i <= nb_dirs; i++) - apply_directive(i) - - # Complete the temp file and atomically rename() it over the - # branch file, so that the latter is always a complete valid - # file, even across a crash. - for (i = 1; i <= nb_lines; i++) { - if (!L_touched[i]) - print L_raw[i] > lock_tmp - else if (L_state[i] != "" || L_has[i]) - print fmt_entry(L_cid[i], L_state[i], L_notes[i], L_has[i]) > lock_tmp - # else: line reduced to nothing, dropped - } - for (i = 1; i <= nb_new; i++) { - if (N_state[i] != "" || N_has[i]) - print fmt_entry(N_cid[i], N_state[i], N_notes[i], N_has[i]) > lock_tmp - } - close(lock_tmp) - system("sync -d " q(lock_tmp) " 2>/dev/null") - if (system("mv -T " q(lock_tmp) " " q(fname) " 2>/dev/null") != 0) { - # our lock was stolen and the temp file went with it: - # what now sits at the lock path belongs to someone - # else, leave it alone and redo the whole cycle from a - # fresh read - lock_held = 0 - renamed = 0 - continue + nb_new = 0; nb_confl = 0 + for (i = 1; i <= nb_dirs; i++) { + if (apply_directive(i)) { + nb_confl++ + CONFL[nb_confl] = d_cid[i] + } } + renamed = 1 + if (nb_confl < nb_dirs) { + # Complete the temp file and atomically rename() it over the + # branch file, so that the latter is always a complete valid + # file, even across a crash. + for (i = 1; i <= nb_lines; i++) { + if (!L_touched[i]) + print L_raw[i] > lock_tmp + else if (L_state[i] != "" || L_has[i]) + print fmt_entry(L_cid[i], L_state[i], L_notes[i], L_has[i]) > lock_tmp + # else: line reduced to nothing, dropped + } + for (i = 1; i <= nb_new; i++) { + if (N_state[i] != "" || N_has[i]) + print fmt_entry(N_cid[i], N_state[i], N_notes[i], N_has[i]) > lock_tmp + } + close(lock_tmp) + system("sync -d " q(lock_tmp) " 2>/dev/null") + if (system("mv -T " q(lock_tmp) " " q(fname) " 2>/dev/null") != 0) { + # our lock was stolen and the temp file went with it: + # what now sits at the lock path belongs to someone + # else, leave it alone and redo the whole cycle from a + # fresh read + lock_held = 0 + renamed = 0 + continue + } - # Commit failures (e.g. missing committer identity) are logged but - # not fatal: the tree stays valid-but-uncommitted and the next writer - # folds it into its own commit. Never checkout/reset here, it would - # eat an admin's uncommitted hand-edit. Git's output is redirected to - # stderr so that it cannot corrupt the CGI response. - if (system("git -C " q(repo) " add -- " q(branch) " 1>&2") != 0 || \ - system("git -C " q(repo) " commit -q -m " q("update " branch) " 1>&2") != 0) - print "update.awk: git commit failed in " repo " (will retry on next write)" > "/dev/stderr" + # Commit failures (e.g. missing committer identity) are logged but + # not fatal: the tree stays valid-but-uncommitted and the next writer + # folds it into its own commit. Never checkout/reset here, it would + # eat an admin's uncommitted hand-edit. Git's output is redirected to + # stderr so that it cannot corrupt the CGI response. + if (system("git -C " q(repo) " add -- " q(branch) " 1>&2") != 0 || \ + system("git -C " q(repo) " commit -q -m " q("update " branch) " 1>&2") != 0) + print "update.awk: git commit failed in " repo " (will retry on next write)" > "/dev/stderr" + } + # else: everything conflicted, nothing changed, nothing to write lock_release() } if (!renamed) die("500 Internal Server Error", "cannot replace branch file") - # echo the resulting line(s) after the status, mostly for debugging + # echo the conflicts then the resulting line(s) after the status; the + # client relies on the "conflict " lines, the rest is mostly for + # debugging. printf "Content-Type: text/plain\r\n\r\n" - printf "OK %d directive%s applied\n", nb_dirs, nb_dirs == 1 ? "" : "s" + printf "OK %d directive%s applied\n", nb_dirs - nb_confl, \ + nb_dirs - nb_confl == 1 ? "" : "s" + for (i = 1; i <= nb_confl; i++) + print "conflict " CONFL[i] for (i = 1; i <= nb_lines; i++) { if (!L_touched[i]) continue