Repository navigation
Conversation
Deployments that generate their config with `piri init` only re-run init when their inputs change, so a new Piri whose init would write something different from the same inputs goes unnoticed: the old config keeps running. That is how staging lost [telemetry] after #131. Add config.GeneratedConfigVersion, bumped only when init's output changes. init records it in the generated config as config_version, and `piri version --config` prints it, so a deployment can compare the two and re-run init when they differ, without re-running it on every upgrade. TestGeneratedConfig pins init's output for a fixture base config that sets every section init reads, keyed by that version. It fails when the output changes under an unchanged version, and -update only records a golden file for a version that has none, so it cannot hide a missing bump. Refs FIL-1223 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ Co-Authored-By: Petra Jaros <peeja@peeja.com>
init writes config.DefaultAggregationConfig(), whose CommP and aggregator job queues default to runtime.NumCPU() workers, and the base config cannot set them. The golden file was recorded on a 4-CPU machine, so the test failed on the 3-CPU macOS runner: workers = 3 against 4. Check that both counts are the host's CPU count, then pin them to the count the golden file was written for. Reproduced with taskset -c 0-2 before the fix; passes with 1, 3 and 4 CPUs after it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ Co-Authored-By: Petra Jaros <peeja@peeja.com>
0d2bd51 to
af84208
Compare
The entrypoints re-ran `piri init` only when the base config changed. Two other things init's output depends on went unnoticed: - init's own arguments, built from node.env: a changed public URL, chain endpoint or registrar never reached the generated config. - the Piri binary. When a base-config change landed before the Piri that understands it, the old init dropped the new section, and upgrading did not re-run init. That is how staging lost [telemetry]. Replace the base-config snapshot with a stamp holding a hash of the base config and init's arguments, and also re-run init when the binary reports a config version (`piri version --config`, fil-forge/piri#138) that differs from the `config_version` in the generated config. An older binary reports none and is left out of the decision. A re-run forced only by a new config version is the one case whose failure is not fatal: the config on disk still reflects every current input, so serving it beats a node that will not start after an upgrade. Everything else still exits, as before. The first boot under this script finds no stamp and re-runs init once. pdp-gate.sh accepts either the stamp or the old snapshot as proof that init has completed on a node. Refs FIL-1223 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ Co-Authored-By: Petra Jaros <peeja@peeja.com>
|
Claude reviewer says… (1st round, Nothing blocking, and the change still fits current Verified:
Generated by Claude Code |
Review of #101 found that the first boot under the stamp, which every node takes right after merge because the bind-mounted entrypoint changes, took the fatal "inputs changed" path: a registrar or RPC blip at that moment would crash-loop Piri. A re-run also had no time bound, and `piri init` ignores SIGTERM, so a hung RPC kept Piri from serving. - Migration: with no stamp yet but the old snapshot still matching the base config, an init failure now warns and serves the existing config, as a version bump does. No config yet stays fatal. - Every re-run (not a first init) runs under `timeout -s KILL` (BusyBox in the Alpine image), 600s by default via PIRI_INIT_TIMEOUT; a timeout warns and serves the existing config if it has a proof set. - Call `piri version --config-version`, the flag's new name in fil-forge/piri#138. - Dev: hash PIRI_PDP_LOTUS_AUTH_TOKEN into the stamp, since init writes it into the config and a rotation must replace it. Correct the compose comment that said it was not written there. Key and wallet contents stay out of the hash on purpose: a wallet-triggered re-init could create a new proof set on chain. - Keep the legacy snapshot instead of deleting it, so a revert of this repo and the old gate still find it; rename the gate's stale snapshot_probe. - Reword the PLC-directory and Postgres-password comments, and add a runbook entry on init re-runs and the WARNING line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
…ding `piri version --config` was a bool that shadowed the root's persistent `--config <path>`: `piri --config /x.toml version` printed the config version, `version --config=/x.toml` failed to parse, `version --help` no longer listed the global flag, and the backticks in its usage made pflag render a value placeholder. The flag is now `--config-version`. Its output is unchanged: the bare number and a newline on stdout, exit 0. version reads no config, so it now skips loading one. Otherwise a malformed default config made it exit 1, which a deployment script reads as a binary that predates the flag. serve now logs a warning when the config records a version other than the one this binary's init writes, and the docs no longer promise that config_version is the first line, which depends on the encoder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
init wrote piri-config.toml with os.Create and returned nil when the create failed, or when a write failed after the file had been truncated. A deployment that reads that file after init (fil-forge/infra-nodes#101 does) could then serve a partial config. The file was also created world-readable under the usual umask, though it holds the lotus auth token and any database and S3 credentials. init now writes it through a temporary file in the same directory (mode 0600, fsynced, then renamed over the old one) and exits non-zero if any step fails. It also no longer echoes the config, secrets included, to a terminal; it prints the file's path instead. Redirected or piped stdout still gets the config, since `piri init ... > config.toml` is the documented way to save it, and a failed write there is now an error too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
…ixture The base-config fixture set only the keys init reads today, so init starting to honour another section (as with [telemetry] in #131) left the golden file unchanged. It now sets every key serve reads, plus every key init's base config parses (principal_mapping included), each to a value init would not write on its own. A new test fails if a serve-side key is missing from it or if it sets a key nothing reads. The test now parses real flags through init's own flag registration and parseAndValidateFlags, with a non-empty lotus auth token, and covers each --network preset with its own golden file, so a preset change also requires a version bump. The steps that need a chain or the network (node setup, registration, delegator proofs) are still replaced by fixed values. The per-CPU worker counts are pinned only when they equal the default, so a count from anywhere else shows up as a golden diff rather than a misleading assertion failure. GeneratedConfigVersion 1 is unreleased, so v1.toml is re-recorded rather than bumped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
|
Claude author says… The fixes are in
Checks:
The PR body is updated. Round 1 had should-fix findings, so a round 2 follows, together with #101's. Generated by Claude Code |
|
Claude reviewer says… (2nd round, Every round-1 finding is fixed as described. Nothing blocking or should-fix; four nits. Verified:
Generated by Claude Code |
TestGeneratedConfigFixtureIsComplete now also checks, for every server config field, that its mapstructure name matches its toml name, since serve decodes what init encodes, and that every fixture value differs from what init writes from an empty base config. The fixture header now credits that test and claims only what it checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
initConfig skipped loading when the version command's flags had been parsed, which stays true for the rest of the process, so any command run after `version` in the same process skipped its config. Config now loads in the root command's PersistentPreRun, which sees the command cobra resolved for this run and skips only version. A test runs another command after `version` and checks that it loads its config. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
When piri-config.toml is a symlink, init now resolves it and replaces the file it points to, keeping the link, rather than replacing the link with a regular file. If writing the config to stdout fails, the error now says the config file was already saved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
The upgrade guide said never to run `piri init` when upgrading, which contradicts the new serve warning. It now says an upgrade doesn't need init unless serve warns the config was generated by a different init, in which case re-running init is safe, and warns that redirecting init's output onto the old config empties it first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
|
Claude author says… All four nits are fixed in
Checks:
This round found only nits, so there won't be another. Generated by Claude Code |
Round 2 of review on #101 found that after a non-fatal init failure (the version or migration path, or a timeout) the stamp or version still differs, so every later start re-ran init: a serve crash, a reboot or a Docker restart each held off serving, and proving, for up to 600s, most of them outside the proving gate. - After a non-fatal failure the entrypoint writes piri-init.failed in the data dir, holding the config version and inputs hash it attempted. While both still match, later starts skip init with one warning and serve the existing config. A new image or changed inputs retry; a successful init removes the marker. Fatal failures write none. - The re-run timeout drops to 300s and is now actually configurable: both compose files pass PIRI_INIT_TIMEOUT from node.env (default 300 when node.env lacks it), and the entrypoints fall back to 300 with a warning on an empty or non-numeric value instead of letting timeout fail before init runs. - Comments made stale by fil-forge/piri#138: the config is now replaced atomically, and init prints it to stdout only when stdout is not a terminal. The container's stdout is a pipe into the shipped log, so >/dev/null stays, for #138's binary as well as older ones. - RUNBOOK: the marker and how to retry, the timeout setting, rollbacks and Postgres password rotations re-running init, exit 137 also being an OOM kill, the migration path hiding a same-deploy input change, and a kill mid-init being recovered by the next run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
Brings in 3bd2957 (refactor: use `syncutil.KeyedMutex`). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Petra Jaros <petra@fil.org> Claude-Session: https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
[From Claude:]
Closes FIL-1368
Problem
Deployments that generate their config with
piri initre-run init only when their own inputs change. The FilOne appliances re-run it when the base config changes. When a new Piri's init would write something different from the same inputs, nothing notices, and the old config keeps running. That's how staging lost[telemetry]after #131: #76's base-config change triggered init under the old binary, which dropped the section, and upgrading to a binary that understands it didn't re-run init.Re-running init on every upgrade would fix that, but then every upgrade would depend on init succeeding (registrar, chain), which isn't true today.
Change
config.GeneratedConfigVersion(pkg/config/full.go), starting at1. It is bumped only when init's output changes for the same base config and flags. A newly honoured base-config section counts, as fix(telemetry): drop the Storacha default collector; carry [telemetry] through init #131's[telemetry]would have, and so does a change to a--networkpreset. Dependency bumps and fix(telemetry): log SDK errors through Piri's logger, backing off repeats #136-style changes don't.initrecords it in the generated config as a top-levelconfig_version. A config written by hand or by an older init has none;FullServerConfig.ConfigVersionis0in that case.servelogs a warning when a config's version is set and differs from the binary's.piri version --config-versionprints just that number and a newline, exit 0, for scripts to compare against the config on disk.versionno longer loads a config file, so a malformed one can't make it fail.piri-config.tomlis now written atomically (temp file in the same directory, mode0600, fsync, rename), and init exits non-zero if that or the stdout write fails. Before, a failed create or write still returned success. Init no longer prints the config, which holds secrets, to a terminal; redirected or piped stdout still gets it, sopiri init ... > config.tomlworks as documented.TestGeneratedConfig(cmd/cli/setup) parses init's real flags throughparseAndValidateFlags, with a non-empty lotus auth token. It runs on a fixture base config that sets every keyservereads, plus every key init's base config parses, each to a value init wouldn't write on its own, and on each--networkpreset. It compares the result withtestdata/generated-config/v<N>.toml(base config) orv<N>-<network>.toml, whereNis the current version. The chain and registrar steps are replaced by fixed values.-updateonly writes a golden file for a version that has none, so it can't be used to hide a missing bump.TestGeneratedConfigFixtureIsCompletefails if the fixture misses a serve-side key or sets one nothing reads.docs/content/configuration/index.mddescribes the field. Theinitdocs describe where the config goes.AGENTS.mdgains a gotcha line pointing at the bump.The deployment side, re-running init when the binary's version differs from the config's, is fil-forge/infra-nodes#101.
Testing
CGO_ENABLED=0 go test -tags skiff ./cmd/... ./pkg/config/... ./pkg/fx/app/...passes with no skips, andgo vetis clean.TestGeneratedConfigpasses repeatedly.TelemetryfromgenerateConfig's output without bumping the version. The test failed with the bump message, and-updatealso failed rather than overwritingv1.toml. I then restored the code. Removing a key from the fixture, or adding a misspelt one, fails the completeness test.TestBaseConfigTelemetryalready round-trips init's output through serve's viper loader, now withconfig_versionin it.1forpiri version --config-version, also with--configpointing at a missing file, and the usual output forpiri version.piri version --helplists the global--config <path>again. A test covers the flag's output with a malformed config.🤖 Generated with Claude Code
https://claude.ai/code/session_01JCF8gyb8jNRmHmWqQmVhRJ
✴️
Generated by Claude Code