Repository navigation
fix(lore-0123): apply the fixes from the review of #137 - #178
Conversation
A failure that lasts (a wrong passphrase or host key, a full box) retried every 20 minutes all day, each try a login to OpenBao and an SSH session to the shared Storage Box. The unit now stops after the run and two retries and stays failed until the next night's timer (D2). With it, from the review of #137: the retry comment says what a retry that meets the lock does (P1), cleanup removes a half-decrypted credential (P5), the header names the retention as it is (P12), and the lock test waits for the holder instead of sleeping, with a case for a lock that is freed (P3, P4).
From the review of #137. The passphrase change stopped only the timer, so a retry waiting to restart could still run in the middle of it; it now stops the service too (P2). Setup step 3 took an SSH or sudo failure for a stored passphrase and said the step was done (P9). "What is where" and Credentials name the backup paths and say the snapshot script decrypts its own credentials on the host (P10). The retry limit of this task leaves the unit failed after three tries, so a start by hand needs `reset-failed` first: said in OpenBao snapshots and done before the passphrase change's last start.
Three tests the review of #137 found missing. The gate's "no facilitator commit" and "uncommitted gate changes" rules sat in gate.ts, which runs the gate when loaded, so nothing tested them. They move into judge() as a required argument, and the local or hosted results folder into resultsFolder(); the list of problems and its order stay the same for every run (P7). The proxy is tested against a facilitator that answers in gzip (P8), and a second call on a channel after a rewrap must land on its first envelope, which pins the sequence the rewrap leaves (P6).
The review of #137 asked 0118 to say how the weekly check and the nightly snapshot share the lock (P11). The time of the check, its wait for the lock and the alarm's age stay 0118's to decide: it gets the facts from the units as this task left them, the three decisions and a criterion for them. 0123's P11 says so.
From the code review of this branch. The test for a freed lock released it on a 300 ms timer, so a slow script could find the lock free and pass without waiting, the kind of timing P3 removed. It now frees the lock only once the script is seen waiting in its own flock. The lock probe takes only flock's exit 1 as "held", and the gzip proxy test sends the compressed length too. The runbook says that stopping the service ends a running snapshot and that a failed first run needs `reset-failed`; 0118 gets the runs that start off the clock and the two texts a second job on the lock changes.
stkrolikiewicz
left a comment
stkrolikiewicz
left a comment
There was a problem hiding this comment.
Three comments: the gate's results folder, the snapshot job's start limit (my answer to Please confirm 1), and the runbook's line on the decrypted credentials.
| * of git. `URL` keeps an IPv6 host's brackets. | ||
| */ | ||
| export function resultsFolder(url: string): "testnet-local" | "testnet-hosted" { | ||
| const local = ["localhost", "127.0.0.1", "[::1]"].includes(new URL(url).hostname); |
There was a problem hiding this comment.
resultsFolder() calls a run local only for three exact hostnames, so any other local
address is filed as hosted: 0.0.0.0, 127.0.0.2, localhost., or a LAN IP (the new test
pins 192.168.1.20 as hosted). The cost is more than a wrong folder name. gate.ts then
rmSyncs results/testnet-hosted/ and writes the local run there, failed or not, so the
committed run of record behind the M1 evidence is replaced in the working tree, one git add
away from a commit.
I'd turn it around: name the hosted facilitator positively (its known host, or an explicit
setting) and file everything else as local. A mistyped URL then costs a gitignored folder, not
the evidence. Fine as a follow-up if you'd rather keep this PR to moves.
There was a problem hiding this comment.
Agreed, and fixed in this PR rather than as a follow-up (f9a0d41). Hosted is now named: HOSTED_FACILITATORS in verdict.ts, today only x402-facilitator.sorobanscan.com, and every other host goes to testnet-local/. That includes the README's own x402.org dry run, which used to land in testnet-hosted/ too. The tests pin our host (also with a slash and in upper case) as hosted, and x402.org, 0.0.0.0, 127.0.0.2, localhost., 192.168.1.20 and a lookalike …sorobanscan.com.example.org as local. A new deployment joins the list in its own PR before its first run of record.
| # session to the shared box every 20 minutes all day. The window is longer than three whole tries, | ||
| # 3 x (TimeoutStartSec + RestartSec), so the count can't run out between them, and shorter than a | ||
| # day, so it never refuses the timer's next run. | ||
| StartLimitIntervalSec=3h |
There was a problem hiding this comment.
Confirming 1, with two corrections.
- The window counts from the first start of a burst, not from the night's run. Three
failed starts in the 3 hours before 03:15 use up the burst, and the timer's own start is
refused withstart-limit-hit. Example: an admin fixes the cause at 01:30 and starts the
job by hand, as the runbook now says to. It fails again at 01:30, 01:50 and 02:10, so the
burst is spent until 04:30. The 03:15 run is refused, that night has no snapshot, and the
alarm fires. The same happens when the server comes back shortly before 03:15 after missing
the previous night's run. So "never refuses the timer's next run" isn't quite true. I'd
reword it to say it can't refuse the next night's run unless the burst started in the
3 hours before it. - A whole try rests on three settings, not two. A try that hits
TimeoutStartSecis
stopped, and the stop can take up toTimeoutStopSec(90 s by default) beforeRestartSec
starts.wholeTryin the units test leaves that out. WithTimeoutStartSec=39minthe test
passes (3 × 59 = 177 < 180), while the real span is 181.5 min: the fourth start lands
outside the window and the retries never stop. Adding the stop time (90 s when the unit
doesn't set it) towholeTrymakes the test guard what the comment says.
There was a problem hiding this comment.
Thanks, both corrections hold, and both are in (f9a0d41).
- The comment now says the window counts from a burst's first start, so a by-hand start that fails again in the 3 hours before the night's run refuses that run. The runbook says the same next to the
reset-failedsteps: clear the limit again after the last failed try. wholeTryaddsTimeoutStopSec, 90 s when the unit sets none. Today that's 3 × 51.5 = 154.5 < 180; yourTimeoutStartSec=39minexample now fails the test (181.5).
| | `/etc/x402/` | the CloudWatch agent's config, the rotation's, the helper's ARNs, the Storage Box's pinned host key | | ||
| | `/srv/x402/backup/creds/*.cred` | the backup jobs' credentials (OpenBao snapshots) | | ||
| | `/srv/x402/backup/creds/*.cred` | the backup jobs' credentials, decrypted by the snapshot script on the host, not for a container | | ||
| | `/run/x402/backup/` | the snapshot job's decrypted credentials and the snapshot, during a run only (tmpfs) | |
There was a problem hiding this comment.
The trap covers every way out but one: a stop that ends in SIGKILL. If a run hangs on a
wedged OpenBao or Docker, TimeoutStartSec stops it, and the trap's bao token revoke and
docker compose exec … rm hang on the same daemon. After TimeoutStopSec, systemd kills bash
partway through the cleanup, and the passphrase, the Storage Box key and the secret-id stay in
/run/x402/backup/ until the next run removes them. That isn't a leak: the files are
root-only, and root can decrypt the .cred files anyway. It does make "during a run only"
here not quite true.
Two small changes: say "removed when the run ends, or by the next run after a kill" here, and
add RuntimeDirectory= to the task's "Not in this task" as the fix at the source. systemd
removes that directory however the process died. The admin borg path runs outside the unit,
so it would need a directory of its own.
There was a problem hiding this comment.
Agreed; both your changes are in (f9a0d41): the row now says the files are removed when the run ends, or by the next run after a kill, and RuntimeDirectory= is in 0123's "Not in this task", with your note on the admin borg path.
One more change on top: cleanup now removes the snapshot file and the credentials first, before bao token revoke and the docker compose exec … rm. A wedged OpenBao or Docker then hangs after the files are gone, so your scenario no longer leaves them; only a kill before the trap runs at all still does.
From Stan's review of #178. The gate filed every host it didn't know as hosted, so a dry run against x402.org, 0.0.0.0 or a LAN machine replaced the committed run of record. Hosted is now named: only the hosts in HOSTED_FACILITATORS, and everything else goes to the git-ignored local folder. The snapshot unit's start limit counts a try's stop time too, and its comment and the runbook say a failed burst in the 3 hours before the night's run refuses that run. Cleanup removes the decrypted files before it calls OpenBao and Docker, which can hang until a kill; RuntimeDirectory= is listed as the fix at the source.
#178 merged: D2, P1-P10 and P12 are done, P11 moved to 0118. Spawn its two follow-ups: 0142 (the snapshot job's decrypted files in a directory systemd removes, and a test of their cleanup) and 0143 (the snapshot AppRole's secret-id expiry, binding and rotation).
Summary
Task 0123: the fixes from the code review of the third promotion (#137), which covered the 55 commits its description had not checked. OpenBao's snapshot job now stops retrying after three tries, its runbook steps match what the job does, and three tests the review found missing are in.
deploy/systemd/x402-openbao-snapshot.service,deploy/libexec/x402-openbao-snapshot): the unit stops after the run and two retries and stays failed until the next night's timer (D2); the retry comment is true (P1);cleanupremoves a half-decrypted credential (P5), and removes the decrypted files before its calls to OpenBao and Docker, which can hang until a kill; the header names the retention as it is (P12).deploy/test/): a units test pins3hand3and that the window is longer than three whole tries and shorter than a day (D2); the lock test waits for the holder instead of sleeping 200 ms, and has a case for a lock that is freed, which frees it only once the script is seen waiting (P3, P4).docs/runbook.md): the passphrase change stops the service as well as the timer (P2); setup step 3 tells an SSH or sudo failure from a stored passphrase (P9); step 5 says what a failed first run needs; "What is where" and Credentials name the backup paths (P10).conformance/src/verdict.ts,gate.ts): the "no facilitator commit" and "uncommitted gate changes" rules move from the gate script intojudge(), as a required argument, and the local or hosted folder intoresultsFolder(), so all three are tested (P7). From Stan's review, hosted is now named: only our deployed facilitator (HOSTED_FACILITATORS), so a dry run or a wrong URL can't replace the run of record. The proxy is tested against a facilitator that answers in gzip (P8).packages/signer-pool/test/settlement-guards.test.ts): a second call on a channel after a rewrap must land on its first envelope (P6). Test only.lore/1-tasks/backlog/): gets the facts and the three decisions on how the weekly check shares the lock with the nightly run (P11).Reviewers, please confirm:
judge()now takes the run's record as a required third argument and holds the two rulesgate.tsused to add after it; the list of problems and its order are the same for every run. One behaviour change in your code, from Stan's review:resultsFolder()files a run as hosted only for a host inHOSTED_FACILITATORS(todayx402-facilitator.sorobanscan.com), not for any host but three local ones; the README's x402.org dry run now goes totestnet-local/. The proxy has one new test and no code change. One older rule moved as it was, for you to judge:gate.tstrims the status column off the first line ofgit status --porcelain.Please confirm
StartLimitIntervalSecis longer than three whole tries,3 x (TimeoutStartSec + TimeoutStopSec + RestartSec)= 154.5 minutes today (Stan: the stop time counts). The units test fails when a change to any of them breaks that. The window counts from a burst's first start, so a by-hand start that fails again in the 3 hours before 03:15 refuses that night's run; the comment and the runbook say so.reset-failed(platform): with the limit, the unit stays failed after three tries, andsystemctl startis refused inside the window. The runbook says so in OpenBao snapshots, and the passphrase change does it before its last start. This is beyond the review's findings; it follows from D2.RuntimeDirectory=is the fix at the source.Testing
pnpm test→ 20 of 20 tasks pass: deploy 113, conformance 44, signer-pool 189, facilitator 1012 (3 skipped), infra 355, mcp 115. That run was before the last two commits; after them,deploy(113) andconformance(50) ran again and pass.pnpm lint,pnpm typecheck,pnpm build→ pass.pnpm format:check→ every tracked file passes.StartLimitlines (D2),channel.sequence = txSequencein the rewrap branch (P6: two envelopes sent, not one), the two record rules injudge()(P7),content-encodingin the proxy'snotForwarded(P8).b6309854. Stan's review is answered inf9a0d418.