Repository navigation
Community feedback: safer saves download, shadow-stamp env mode, maintenance mode, release prep - #2
Merged
Merged
Conversation
…tenance flag #1 — Saves download snapshot: /api/saves/download used to tarfile.add() straight from the live R5/Saved tree. That races with RocksDB compactions (.log → .sst renames mid-walk), and on memory/IO-pressured hosts has been reported to corrupt the DB — symptom: world fails to load on next boot, server generates a fresh one. Now snapshot to a /tmp TemporaryDirectory via shutil.copytree first, tar the scratch copy, stream that. Download I/O is decoupled from the live DB entirely. #2 — Shadow-stamp env mode: env-mode reconcile_server_config used to re-stamp ServerName / MaxPlayerCount / P2P / etc. on every boot, blowing away UI config-editor edits. Now writes ServerDescription.last-env-stamp.json after each stamp; the next boot only re-stamps a key whose current value still matches the shadow. If the value diverges (UI edit, ldb, manual jq), the entrypoint logs "keeping operator-modified <key>" and leaves it alone. First-boot (no shadow) stamps everything. #3 — Maintenance flag: .maintenance-mode file at R5/ makes the entrypoint loop-sleep instead of launching Proton. UI card toggles it via the new /api/maintenance GET/POST pair; POST accepts {active, restart?} so "enable + stop now" and "disable + restart now" flow through a single interaction. Handler traps SIGTERM so the container still exits cleanly on shutdown. Server Config card blurb now mentions the shadow-stamp behavior so operators know env-mode edits survive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PR's first-pass shadow-stamp refresh did `cp live → shadow`, which captures an operator's divergent value into the shadow. Next boot, live == shadow (both have the operator value) → we'd stamp over their edit with the env value. Clobber on the second boot after divergence, not the first — subtle but user-visible. Fix: after running the jq filter, build a new shadow via jq-merge rather than whole-file copy. For each managed key, write the env value iff we stamped it THIS boot; otherwise preserve the old shadow value. First boot (no shadow present) seeds every key. Verified with a 3-boot loop: operator edit on boot 1 survives boots 2 AND 3 (old diff only got boot 2 right). Same edit drops the unused `env_val` param from `_env_keep_key` (SC2034 — what CI actually flagged; the signature was a leftover from a comparison strategy I didn't end up using). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Upgrade case: operators adopting this change on top of an existing install don't have a shadow file yet. The previous logic (_env_keep_key returning "false" on first_boot) stamped the live file with env values on that first post-upgrade boot — which clobbered, e.g., a ServerName the operator had already customized via ldb or the UI. New behavior: on first_boot, _env_keep_key returns "true" (skip) for every managed key. No stamping happens. The shadow still gets seeded with env values (via the $seed branch in the final jq merge), so on boot 2 we have a baseline: if live still matches env intent, we stamp; if operator diverged, we preserve. Log: - First-boot skip-all path gets one "no shadow — treating live as operator-owned" line instead of a per-key "keeping" spam. - Normal path still logs per-key on divergence. Validated locally: - Upgrade (pre-existing "Digital Ocean Canary", env default "Windrose Server"): live preserved across boots 1, 2, 3. - Fresh install: env change on boot 2 takes effect as expected. - PersistentServerId preserved (not a managed key). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5ff4a3ea4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # stamp normally; if operator diverged, we preserve. | ||
| _env_keep_key() { | ||
| local path="$1" kind="$2" | ||
| if [ "${first_boot}" = "true" ]; then echo "true"; return; fi |
There was a problem hiding this comment.
In env mode, a missing shadow file makes _env_keep_key unconditionally return true (skip), so none of the managed fields are stamped on that boot. If ServerDescription.json differs from env at that point (for example custom SERVER_NAME/MAX_PLAYER_COUNT), the subsequent shadow seed records env values, and future boots see live != shadow and keep treating those keys as operator-modified forever. That means env-driven config can stop applying entirely on installs that start without ServerDescription.last-env-stamp.json.
Useful? React with 👍 / 👎.
Comment on lines
+547
to
+548
| | (if $s_invite == "true" then .InviteCode = $invite else . end) | ||
| | (if $s_island == "true" then .WorldIslandId = $islandId else . end) |
There was a problem hiding this comment.
The shadow-refresh block states first boot seeds every key, but InviteCode and WorldIslandId are only written when stamp_* == "true" and are not covered by $seed. Since first boot currently skips all stamps, those two fields are omitted from the shadow; on later boots the comparison becomes live vs null, so INVITE_CODE and WORLD_ISLAND_ID are perpetually considered diverged and never re-applied from env.
Useful? React with 👍 / 👎.
PR #2 shipped shadow-stamp for ServerDescription but missed the sibling ensure_world_layout path, which unconditionally re-stamps WorldName + WorldPresetType on every env-mode boot. Reported live: operator changes the world name via UI, restarts, name snaps back to the env default on boot. Apply the same shadow-stamp treatment to WorldDescription per island — shadow at `WorldDescription.last-env-stamp.json` next to the live file, first-boot seeds from env intent without stamping, later boots compare live to shadow and skip when diverged. Also land the review nits from the first PR #2 review pass: - Shadow file corruption guard in reconcile_server_config: `jq empty` the shadow before using it as --argjson base. Malformed JSON (partial write, manual edit gone wrong) used to exit the entrypoint under `set -e` and boot-loop the service. Now we log a warning and reseed as if first boot. - WINDROSE_DOWNLOAD_SCRATCH_DIR env lets operators point /api/saves/download's copytree scratch at a PVC-backed path when /tmp is tmpfs (RHEL default, some Docker setups). Defaults to /tmp, which is fine on most hosts. - helm/windrose/README.md "Server Config Modes" expanded: mutable mode now has a step-by-step recipe, and the env/mutable distinction calls out that env-mode preserves UI edits via the shadow-stamp, so mutable is only needed for the stronger "entrypoint never touches the file" guarantee. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Community-reported observation + logs: SteamCMD's app_update intermittently reverts the idle-CPU patch on restart (it detects the modified bytes don't match its local manifest and "corrects" them). The entrypoint then re-applies the patch via maybe_patch_idle_cpu. Net effect is correct final state, but every reboot paid a SteamCMD run + patch re-apply cycle, and there was a short window where the binary was actually unpatched. Fix: if the effective patch mode is ON (env=1 or override=enabled) and the binary is already in "patched" state, skip the SteamCMD step entirely. Uses --print-state for the check (fast path on known MD5s, no file I/O beyond a few targeted reads). Trade-off: Windrose game updates won't be pulled on boot when the patch is active — operator has to set WINDROSE_STEAMCMD_FORCE=1 for that one boot to refresh. Game updates are rare (monthly at most); worth the quiet boots in exchange. Documented in README env table. Partial-install detection (binary missing, dir has content) still triggers validate, so first-boot/fresh-PVC scenarios are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This reverts commit 20446ab.
…place
Old model: mutate the Steam-managed EXE in place. Problem: SteamCMD's
app_update intermittently reverts the modification back to the manifest
version, so every boot paid a "Steam reverts → we re-patch" thrash, and
there was a short window where the live binary was unpatched.
New model: leave the original EXE untouched and maintain a sibling
copy named `WindroseServer-Win64-Shipping.patched.exe` alongside it,
with `WindroseServer-Win64-Shipping.patched-source.md5` recording the
source md5 it was built from. On each boot:
1. maybe_patch_idle_cpu md5s the source.
2. If the sibling exists AND the recorded source md5 matches → reuse.
3. Else rebuild: cp source → sibling, run patch-idle-cpu.py against
the sibling, write the new source md5 alongside.
4. Export IDLE_PATCH_EFFECTIVE_EXE pointing at the sibling.
5. exec proton launches IDLE_PATCH_EFFECTIVE_EXE instead of the
original.
SteamCMD operates on the original as always and has nothing to
"correct". Updates are picked up automatically — next boot's source
md5 changes, the sibling rebuilds. If a future Windrose version
breaks the patch script's signature scan, the rebuild fails, the
broken sibling is removed, and we fall back to launching the
original with a WARNING log — "until we can't" behavior the user
asked for explicitly.
UI changes:
- idle_patch_binary_state() no longer subprocess-calls the script.
Checks for the sibling + reads the source-md5 file. Cache keyed on
(source mtime+size, sibling mtime). New "stale" state indicates
the source changed since the sibling was built (will rebuild on
next restart).
Effective-OFF path (env=0 or override=disabled) just removes the
sibling; no more --revert --idempotent dance needed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hosts that were running the previous in-place patch model have their Steam-managed original EXE patched directly. The new sibling flow tries to cp that original → sibling and then patch the copy, but the script refuses to re-patch an already-patched binary and errors out, falling back to launching the original (which happens to still be patched, so it works by accident). Fix: before building the sibling, run --revert --idempotent on the original. On a host migrating from the old model it un-patches the original cleanly; on a fresh host it's a no-op. Now the sibling-build flow gets a clean copy to work with, and Steam's view of the file matches its manifest going forward. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous commit added a --revert --idempotent pre-step to clean the original EXE on hosts upgrading from the old in-place model. That was over-engineered — the patch script's --idempotent flag already handles the degenerate "cp copied an already-patched binary" case: it detects the patched state, logs "info: already patched", and exits 0. New flow for the upgrade host (original still patched from old model): 1. cp original (patched) → sibling (patched) 2. patch-idle-cpu.py sibling --idempotent → exit 0, no-op 3. record the patched md5 as cached source md5 4. launch sibling (patched; server works normally) SteamCMD on a later boot may restore the original to its manifest version. At that point the cached md5 no longer matches → rebuild from clean bytes → records the unpatched md5 as the new cached source md5. Self-heals. Original EXE is now never modified by our code. Steam owns it end to end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wineserver detaches to ppid=1 under Proton's startup path, which means
systemd's cgroup-level SIGTERM on service restart can't always reach
it. On the next start the new proton sits in waitforexitandrun behind
the stale wineserver, and the user sees the old game process (still
holding the pre-patch code in memory) continuing to burn CPU.
Fix: tag every process we spawn with a boot-unique env var
WINDROSE_INSTANCE_TAG, written to a pidfile at
${WINDROSE_PATH}/.last-instance-tag. On next boot, scan /proc/*/environ
for matching tags and SIGKILL those. Scoped per-install via
WINDROSE_PATH (separate installs read separate pidfiles) and per-boot
via "$$-$(date +%s)" (fresh tag each time), so multi-tenant hosts with
shared binaries + per-instance save roots stay safe — we only match
processes we ourselves launched on the last boot.
No effect on k8s/Docker (separate PID namespaces mean no stale procs
to find). Main beneficiary is bare-Linux + compose-pid-host, and
anyone migrating from the old in-place patch model where stale
processes held the old unpatched code in memory.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- scripts/entrypoint.sh: split the export + assign on WINDROSE_INSTANCE_TAG so the subshell exit status isn't masked (shellcheck SC2155). - bare-linux/install.sh: after enable --now, call try-restart on windrose-ui + windrose-game. Without this, re-running install.sh to pick up updated UI/entrypoint code was a no-op at the process level — the services were already running, enable --now didn't bounce them, and the Python UI kept the old server.py in memory. try-restart only bounces services that were already active, so first installs aren't double-started. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ness
Extends backup retention beyond the bare count cap to fix the
4-18/4-19 data-loss footgun: a burst of test backups today pushed
the user's actual known-good snapshots out of the 5-slot retention
window, and the restore endpoint's only fallback data was the
Apr-19 backup by sheer luck that it hadn't been evicted yet.
Three retention rules, evaluated in create_backup(); a backup
survives if ANY rule keeps it:
1. Name starts with `manual-` (pinned; never pruned).
2. In the top-N most recent by name-sort (count-based, default 5).
3. Younger than WINDROSE_BACKUP_RETAIN_DAYS (default 7).
Rule 3 is the new one — handles the "busy week pushed out my
weekly snapshot" case. Rule 1 via create_backup(pin=True) / POST
/api/backups {"pin": true} / POST /api/backups/{id}/pin.
Two new endpoints: /api/backups/{id}/pin + .../unpin for toggling
the prefix on existing backups without re-creating.
Docstrings on create_backup() / restore_backup() explicitly warn
never to use raw `cp` for recovery — partial-subtree swaps of
Saved/.../Worlds/<id>/ leave game-internal state pointing at
stale files. This was the concrete mistake that turned a bad
restore attempt into a crashloop today.
Data-loss prevention at the entrypoint:
- preserve_dir moved from /tmp to $WINDROSE_PATH/.steamcmd-preserve-$$
so a mid-SteamCMD container kill doesn't throw the stash away with
/tmp on container restart. Abandoned preserve_dirs are detected
and restored at entrypoint start.
- ensure_world_layout refuses to bootstrap a fresh world if a
.had-world-once marker is present and Saved/ is empty, unless
WINDROSE_ALLOW_FRESH_WORLD=1. Logs a LOUD error pointing the
operator at backups + the stray preserve_dir they might recover
from. Would have caught today's silent bootstrap at 16:34.
CI: new python-tests job runs scripts/ui/test_retention.py — 10
stdlib-only unit tests covering the retention logic, including a
reproduction of today's burst-evicts-older-backup case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- entrypoint.sh: drop the unused had_saved local in install_via_steamcmd. It was a leftover from an earlier intent I didn't follow through on; the abandoned-preserve_dir recovery at entrypoint-start already covers the interrupted-stash case without needing this flag. - test_retention.py: fake_stat signature needs to accept *args/**kwargs so Path.stat(follow_symlinks=...) calls in Python 3.13+ dispatch through without TypeError. CI runs on 3.13; my local 3.12 didn't hit it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Python 3.13 stopped silently catching OSError inside Path.is_dir(),
so a single unreadable entry during iterdir() was aborting the entire
_prune_backups() sweep — and the matching list_backups() enumeration.
My local Python (3.14) didn't expose this on my first pass; CI's 3.13
caught it.
Wrap each is_dir() call in a try/except OSError: pass so the
iteration keeps going past unreadable dirs. Matches the sensible pre-
3.13 default without relying on it.
test_retention.py's unreadable-dir test was already asserting the
right behavior ("unreadable entries must not be pruned"); it was the
production code that was wrong. Test now verifies the real fix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two new stdlib-only test files wired into CI's python-tests job: - test_restore.py (6 scenarios): create_backup → restore_backup round-trip, including mutations-wiped-by-restore, post-backup files get removed, identity JSONs restored, missing-id raises, and RocksDB bytes survive byte-for-byte. Fills the gap where restore_backup had no direct coverage — the primitive that today's incident pivoted on. - test_http.py (7 scenarios): binds Handler to an ephemeral localhost port in-process and exercises POST /api/backups, /pin, /unpin, /restore via urllib. Covers routing, JSON body parsing, 401 on destructive endpoints without auth, 404 on unknown backup id, and basic-auth round-trip. No subprocess, no network beyond loopback. Both run in <1s locally on Python 3.14. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two intertwined changes that landed together before the per-task commit
rule took effect. Future work follows one commit per task.
Auto-backup scheduler (task #43):
- Idle trigger: N min after last player disconnects, fires once per
zero-streak, resets when players return. Default 1 min.
- Floor trigger: M h of continuous active session without an auto-backup
forces one. Default 6 h. Zero on either disables that trigger.
- Manual backups don't reset auto clocks (independent systems, like
in-game manual saves vs auto-saves).
- Config single source of truth at $R5_DIR/.backup-config.json;
WINDROSE_AUTO_BACKUP_* env vars only seed defaults until the file
exists. Atomic temp-file-rename on UI save.
- Retention count default bumped 5 → 10 (hard floor even if scheduler
stalls). Age + count are OR-combined — age never subtracts.
- .auto marker file + source=auto/manual/manual-pinned tag in
list_backups + webhook payload includes source + reason.
- Bootstrap re-derives lastAutoBackupAt from newest .auto-marked dir
so UI container restarts don't cause spurious immediate backups.
- New GET/PUT /api/backup-config with range + type validation.
Default_Backups tab (task #40):
- GET /api/game-backups + POST /api/game-backups/{ts}/restore.
- Restore merges onto live RocksDB tree and creates a pinned safety
snapshot first. Clearly labeled as world-only in UI + confirm dialog
(game backups don't include ServerDescription/WorldDescription).
- Tabs in the Backups card: Ours | Default_Backups (game).
Backup payload widened to also capture .backup-config.json and
.idle-patch-override so operator-owned runtime settings survive a
restore. Staged configs intentionally excluded.
Tests: 38 total passing across retention/restore/scheduler/HTTP suites.
New test_auto_backup.py wired into CI.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pure move — no behavior change. Rationale:
- server.py is the backend; ui/ is the frontend bundle — colocating
them under scripts/ui/ misrepresented their different roles.
- Tests in /tests/ is Python-idiomatic and keeps them out of the
deployment copy targets (Dockerfile, install.sh).
- Once server.py grows past one file we can convert to a package;
flat root supports either shape.
Changes:
- scripts/ui/server.py → server.py (STATIC_DIR now resolves to
Path(__file__).parent / "ui", with UI_STATIC_DIR env override for
nginx-front deployments).
- scripts/ui/{index.html,app.css,app.js} → ui/
- scripts/ui/test_*.py + test_api.sh → tests/
- Dockerfile: COPY server.py and ui/ into /opt/windrose-ui/
separately; k8s invocation path unchanged.
- CI: point py_compile + test invocations at new paths.
- bare-Linux install.sh: lay down server.py at ${WINDROSE_INSTALL_DIR}
root with ui/ sibling; /opt/windrose-ui symlink now targets the
install dir itself so /opt/windrose-ui/{server.py,ui/} both resolve;
cleans up any stale scripts/ui/ from prior installs.
- server.py PATCH_SCRIPT_CANDIDATES updated (server.py is now repo-
root, so patch-idle-cpu.py is parent/scripts/, not parent.parent).
- Docs (README, CLAUDE.md, bare-Linux README, TODO, values.yaml
comments) refreshed to reflect new paths.
Tests: all 38 pass under the new layout.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
seslly
changed the title
The previous dockerignore whitelisted only scripts/ and the Dockerfile. After the repo flattened (server.py + ui/ at root), the Docker build started failing with "/ui": not found because the COPY targets weren't in the build context. Add !server.py, !ui, !ui/** to the whitelist so the COPYs resolve. Verified locally: docker build now completes through every COPY step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Community feedback asked for a real Start button on bare-Linux. The
only way to do that cleanly is to let the UI drive systemd directly —
previously the UI could only SIGTERM the process and rely on the
container supervisor to bring it back.
New pieces:
- bare-linux/polkit/50-windrose.rules: narrowly grants the 'steam'
user start/stop/restart on the windrose-{game,xvfb,ui}.service
units only. Scoped to org.freedesktop.systemd1.manage-units (NOT
manage-unit-files — no enable/disable/mask escalation). Other
systemd actions + other users fall through to default polkit.
- install.sh: drops the rule at /etc/polkit-1/rules.d/50-windrose.rules
when polkit is present, skips cleanly otherwise (UI falls back to
SIGTERM).
- server.py: _systemctl_available() + systemctl_dispatch() helpers;
existing /api/server/stop + /api/server/restart prefer systemctl
when available, fall back to SIGTERM if polkit isn't configured or
the call fails — operators in a half-broken state still see
*something* happen.
- New POST /api/server/start — only available when systemctl is
usable (501 on k8s/compose where the supervisor owns lifecycle).
- /api/status exposes serverControlMode ("systemctl" | "signal") so
the UI can show/hide the Start button intelligently.
- UI: Stop + Start buttons in the toolbar, visibility + enabled state
driven off serverControlMode and serverRunning.
Tests: new tests/test_server_control.py with 6 cases covering
detection logic + subprocess dispatch (monkey-patched shutil.which
and subprocess.run so CI doesn't need real systemd).
Docs: CLAUDE.md API table updated.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Windrose 2026-04 added Direct IP Connection — a bypass of the backend
connectivity service where clients join via a raw IP:port rather than
an invite code. Useful when an ISP blocks Windrose's backend or for
LAN-only play. Live ServerDescription.json shape (confirmed on prod):
UseDirectConnection: bool
DirectConnectionServerAddress: string
DirectConnectionServerPort: int (default 7777)
DirectConnectionProxyAddress: string (default 0.0.0.0)
Plumbing:
- entrypoint.sh env defaults + reconcile_server_config stamp blocks for
all four fields. Shadow-stamp treats the whole Direct-IP bundle as
opt-in via USE_DIRECT_CONNECTION presence — unset env leaves existing
configs alone so upgrades don't silently flip anyone into Direct IP
mode. Empty address with USE_DIRECT_CONNECTION=true transparently
reuses P2P_PROXY_ADDRESS (same "my IP" from the host's perspective).
- helm/values.yaml: serverConfig.useDirectConnection (null | bool) +
serverConfig.directConnection.{serverAddress,serverPort,proxyAddress}
wrapped in null-guard so env vars only render when explicitly set.
- docker-compose.yaml + bare-linux/install.sh: new env keys in the
_MANAGED_KEYS list and default env file, empty by default.
- server.py validate_server_description: four new optional fields +
a range check on port + a required-if-enabled rule on the address
(empty address with UseDirectConnection=true is the silent-bounce
footgun called out in the release notes).
- ui/: Advanced > Direct IP Connection collapsible section with all
four inputs. Checkbox + 3 text/number fields, synced two-way with
the Raw JSON editor like the rest of the form.
Tests: new tests/test_schema.py with 6 cases covering baseline + valid
enabled + rejected empty-address + disabled-with-empty-address OK +
wrong types + port range. Wired into CI. Full suite (46 tests) passes.
Helm template verified: --set useDirectConnection=true renders all
four env vars; default template omits them entirely.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Community feedback flagged that operators weren't sure which restore
path to reach for when things went sideways. The 16:34 incident earlier
in this PR was a data point: the right answer is never "raw-cp parts
of a backup onto a live tree" — it's restore_backup() through the API.
New ## Recover Your World section, placed between the game-patch update
flow and the invite-code snippet (both are operator recovery/lookup
flows). Covers four recovery paths in order of lightness:
1. UI backup restore — the default, whole-tree + identity JSONs,
preserves PSID so backend rebinds to the same island
2. Default_Backups merge — world-only, takes a pinned safety snapshot
first, useful when a game auto-backup is more recent than any UI
backup
3. Disaster mode — on-disk cp recipe if the UI is crash-looping,
mirrors exactly what restore_backup() does programmatically
4. Lost PSID / orphaned island — only path #1 fixes this; game
Default_Backups won't help since they don't capture identity
Plus Prevention (pin before risky ops, retention math, watching auto
drift, why /tmp is a footgun) and What NOT to do (raw-cp world
subtree, delete ServerDescription, race the scheduler with a direct
file write).
No code changes, doc-only. CLAUDE.md is a symlink to README.md so the
agent-facing copy gets this too.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Regression caught on the VPS rollout (2026-04-21): re-running
install.sh dropped UI_PASSWORD from the env file. Operator's password
was on disk, correctly read into the merge loop, but clobbered back
to empty before the heredoc wrote it out.
Root cause: install.sh pre-assigned managed-key defaults BEFORE the
merge loop:
UI_PASSWORD="${UI_PASSWORD:-}" # <- this
...
while IFS='=' read -r _k _v; do
if [ -z "${!_k+x}" ]; then # "is var unset?" — always false now
printf -v "$_k" '%s' "$_v" # ... so this skips
fi
done < existing.env
The [-z "${!_k+x}"] check was meant as "did the operator pass this on
the CLI?" — but the pre-merge ":-" assignment always SET the var
(possibly to empty), so the merge could no longer distinguish "CLI
gave me empty" from "no CLI, default fallback kicked in", and fell
back to skipping every managed key. UI_PASSWORD, the webhook URLs,
UI_ENABLE_ADMIN_WITHOUT_PASSWORD etc. all lost their on-disk values.
Fix: don't pre-assign defaults for managed keys. Move every ${:-...}
default inline into the heredoc so they apply only AFTER the merge
has had a chance to populate from the existing file. The
UI_BIND=0.0.0.0 warning check at the top uses inline ${:-defaults}
so it still works pre-merge without re-introducing the bug.
Regression test: tests/test_install_env_merge.sh extracts the
merge-loop + heredoc, runs it against a simulated pre-existing env
file holding a real UI_PASSWORD + webhook URL + operator extras,
and asserts every one survives. Verified the test catches the
original bug: reverting the fix makes it FAIL with "UI_PASSWORD: want
s3cretOperatorValue got ''" — exactly the symptom we hit on VPS.
Wired into .github/workflows/ci.yml so this can't silently regress.
Re-ran install.sh on the VPS with the fix in place: password
preserved across two consecutive runs, 401/200 auth behavior matches
expectations. No other operator got hit because the VPS was the only
host rolled through the broken install.sh today — prod k8s uses the
Dockerfile + helm path and was never affected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n honest knob Bug caught on prod rollout (2026-04-21): operator set launchArgs="-FPS=120" on the game command line, expecting `stat srvfps` to bump from 30 to 120. It stayed at 30 — `-FPS=N` only caps the render loop; the server-side tick rate is NetServerMaxTickRate in Engine.ini. Root cause: the entrypoint's Engine.ini seed was seed-once. If the file existed (which it did on every non-pristine install), NET_SERVER_MAX_TICK_RATE was silently ignored. Our documented env knob was doing nothing on any deployment that had ever booted. Prod shipped with NetServerMaxTickRate=30 pinned for ~72h because of this. Fix: - scripts/reconcile-engine-ini.sh: new sibling script, exported from the monolithic entrypoint so it's unit-testable. Stamps NetServerMaxTickRate in both driver sections (UR5NetDriver + stock IpNetDriver belt-and- suspenders) and t.MaxFPS in [ConsoleVariables] — t.MaxFPS caps the main loop, which on a dedicated server with no rendering is what actually drives the game tick, so it must track the net tick rate. - Uses shadow-stamp: writes $R5/.engine-ini-shadow recording the value we last wrote. Next reconcile compares live vs shadow — matching means "operator hasn't touched it, update to new env value"; divergence means "operator hand-edited, leave alone". Same pattern as the ServerDescription reconcile block. - entrypoint.sh delegates to the script; old seed-once block deleted. - Dockerfile + bare-linux/install.sh: install the script at /usr/local/bin/reconcile-engine-ini.sh. - Helm: new serverConfig.networkTickRate value (default 60) plumbed into NET_SERVER_MAX_TICK_RATE on the container. - values-local.yaml (prod): networkTickRate=120 to match launchArgs=-FPS=120. - docker-compose + bare-linux env: new NET_SERVER_MAX_TICK_RATE knob. - README env table: new row explaining this IS the tick rate knob, plus operator guidance on when to raise (120 on beefy hosts for smoother combat) vs lower (30 on 1-2 vCPU VPSes to cut CPU/bandwidth). tests/test_engine_ini_reconcile.sh: 7 cases covering env-unset no-op, fresh seed, the prod repro (stale existing file + no shadow → reconcile), shadow-match reconcile, operator-override preservation, idempotent same- value, and unrelated-keys-preserved across reconcile. Wired into CI. Verified the test fails on the old seed-once logic — exact regression signature we hit today. Rollout: canary upgraded (60→120 reconcile logged), then prod (absent→120 on [ConsoleVariables] + both driver sections reconciled to 120). Game logs confirm "LogConfig: Set CVar [[t.MaxFPS:120]]" on first boot under the new image. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Community-feedback final gap: we had /api/saves/download but no way to
push a backup back onto a host. Operators wanting to move a server
(home → VPS, node swap, DR rehearsal) had to kubectl cp or SSH in.
And /api/saves/download was saves-only — no ServerDescription.json —
so even with a shelled-in upload, PSID was lost and the backend
rebound to a fresh island.
Two new endpoints, symmetric with the existing backup primitive:
- GET /api/backups/{id}/download — streams the whole backup dir (Saved/
+ ServerDescription + WorldDescription + .backup-config + .idle-patch-
override) as .tar.gz. Filename is windrose-backup-{id}.tar.gz. Safe
to tar in-place (backup dirs are immutable post-create_backup).
- POST /api/backups/upload — accepts that exact shape, extracts into a
new manual-imported-<ts>/ dir (pinned by prefix — operator explicitly
brought it over, retention shouldn't evict), returns the id so caller
follows with /api/backups/{id}/restore. Accepts tar.gz / tar / zip by
magic bytes + filename hint (same extract_archive routing as
/api/upload). Strict validation — rejects 400 when the archive is
malformed, missing Saved/, or junk bytes.
UI:
- Per-row Download button in the Backups card — fetches via authFetch
(not raw <a href>) so basic-auth creds + XHR-suppress logic both work.
Blob URL + synthetic click fires the browser's Save As dialog.
- Import control next to Create backup now: file input + Upload+pin
button. Status line shows the imported id so operator knows what
row to Restore.
4 new HTTP integration tests:
- download → upload → restore round-trip, with byte-level assertion on
RocksDB MANIFEST contents + identity JSON content preservation
- 404 on unknown backup id
- 400 on non-archive garbage bytes (with the ReadError → ValueError
coercion that lets the handler return 400 instead of 500)
- 400 on structurally-valid tar missing Saved/ (so a malformed backup
doesn't land on disk and silently break a later restore)
Docs:
- README "Recover Your World" gets a new "5. Migrate to a new host"
section with UI flow + curl recipe covering all three deployment
surfaces.
- CLAUDE.md API table gets the two new rows.
E2E validation (prod → canary, API only, no kubectl cp): downloaded a
4.6 MB backup from prod (invite c030f708, island 7A0A41E...), uploaded
to canary as manual-imported-20260421T044813Z, restored. Canary's
invite + worldIslandId post-restore matched prod's exactly (PSID
preserved through the tarball → backend rebound to the same island).
Canary then restored back to its own pre-test safety snapshot so the
two environments stay separate.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c343e85bba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+352
to
+353
| UI_BIND=${UI_BIND:-127.0.0.1} | ||
| UI_PORT=${UI_PORT:-28080} |
There was a problem hiding this comment.
On a fresh bare-linux install (no existing env file and no CLI overrides), this expansion only writes defaults into the generated env file; it does not assign shell variables in-process. Because the script runs with set -u, later status output that dereferences ${UI_BIND}/${UI_PORT} aborts with an unbound-variable error, causing install.sh to exit non-zero after doing the install work.
Useful? React with 👍 / 👎.
| payload = json.loads(body) if body else {} | ||
| except json.JSONDecodeError: | ||
| self._send(HTTPStatus.BAD_REQUEST, "text/plain", b"invalid JSON body\n"); return | ||
| want_active = bool(payload.get("active")) |
There was a problem hiding this comment.
Casting with bool(payload.get("active")) means any non-empty JSON value (for example "false") is treated as true, so malformed-but-common clients can unintentionally enable maintenance mode; payload.get("restart") has the same truthiness issue for restart signaling. This makes the API behavior surprising and unsafe unless callers always send strict JSON booleans.
Useful? React with 👍 / 👎.
Two UI nits surfaced during the post-rollout review: 1. Backup list sort was name-based, which groups the "manual-*" and "manual-imported-*" prefixes lexicographically separately from the bare-timestamp auto entries. Result: a pinned backup from last week appears above an auto-backup from five minutes ago, even though the column reads "ID (UTC)". Fix: sort list_backups() + list_game_backups() by mtime (true chronological) before rendering. 2. body had `margin: 2rem; max-width: 60rem` hardcoded — cramped on phones (huge wasted margins on narrow viewports) and a postage stamp on widescreens. Replaced with `padding: clamp(0.75rem, 2.5vw, 2rem); max-width: 80rem; margin: 0 auto` so content breathes at any size without needing per-breakpoint media queries. Also added flex-wrap to .row (button rows no longer overflow) and overflow-x:auto on .card (tables scroll inside their card on narrow viewports instead of blowing out the page). Deployed to canary + eyeballed at phone, laptop, and ultrawide widths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses all four comments from @codex on PR #2: [P1] entrypoint.sh env-mode shadow seeded env instead of live on first boot. Meant _env_keep_key stayed "skip forever" whenever the on-disk ServerDescription differed from env intent on the first boot after install — env mode silently stopped applying any managed keys from the second boot onward. Fix: on first boot, seed the shadow from the LIVE file (snapshot of whatever is on disk). Next boot's live==shadow comparison can then correctly stamp env when no operator drift, or preserve when there is. Side effect: since the shadow now holds real live values after first boot, InviteCode and WorldIslandId are captured too — no more "permanently diverged, never re-applied from env" for those two keys (P2 from the same review, subsumed by the P1 fix). Corrupt shadow handling: instead of silently treating as first-boot (which would re-seed with env values and repeat the bug in slow motion), we now explicitly log + delete + return — next boot goes through the clean first-boot branch and re-seeds from live. [P1] bare-linux/install.sh status echo was dereferencing UI_BIND and UI_PORT under set -u. After the earlier install.sh env-merge fix removed pre-merge :- defaults (to make operator passwords survive re-runs), a fresh install with no existing env file + no CLI overrides left those shell vars unset — the final status block aborted the script with "unbound variable". Fix: add post-merge := default assignments right before the heredoc, with a comment pointing at the earlier bug so the next person doesn't revert the post-merge block thinking it's the one that wipes passwords. [P2] server.py maintenance POST used bool(payload.get("active")) which treats any non-empty JSON value (including the STRING "false") as truthy. A misconfigured client could flip maintenance mode on by mistake. Fix: require real JSON booleans for both "active" and "restart" fields; 400 otherwise. "active" is required, "restart" is optional (defaults false). Tests: - tests/test_shadow_stamp.sh: 3 scenarios covering first-boot shadow-matches-live, second-boot-stamps-on-no-drift, and second-boot-preserves-on-operator-drift. Extracts the jq reconcile directly so any drift between test and entrypoint.sh shows up as a test failure. - tests/test_install_env_merge.sh: extended with a fresh-install set -u test that fails if any of UI_BIND / UI_PORT / UI_PASSWORD / UI_ENABLE_ADMIN_WITHOUT_PASSWORD end up unset after the merge block. - tests/test_http.py: new test_maintenance_requires_strict_bool covering the 5 bad-shape inputs the old code accepted + 3 good shapes that must still succeed. Also extends _TestServer's module-level patching to include MAINTENANCE_FLAG_FILE so the test doesn't try to touch the host's /home/steam. All suites pass. Shadow-stamp test also added to CI workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
seslly
approved these changes
Apr 21, 2026
seslly
left a comment
seslly
left a comment
Contributor
There was a problem hiding this comment.
tested in canary k8s and digital ocean
seslly
added a commit
that referenced
this pull request
Apr 28, 2026
…laky User-reported fresh `docker compose up` failed with: ERROR! Failed to install app '4129620' (Missing configuration) ERROR: SteamCMD exited with 8 Root cause (verified by clean-container A/B): SteamCMD's anonymous app_update is flaky on the FIRST attempt within a container, then succeeds reliably on attempt #2 with no other change. Probably a Steam-side anonymous-license caching quirk that warms up after the first call. Fresh containers always hit attempt #1. Reproduced cleanly: cold attempt #1 FAILs, #2 OK + starts downloading, #3 resumes #2's partial download at ~50-65%. The depots are public anonymous regardless, the credentials are valid, the manifest is present — Steam just returns the cryptic "Missing configuration" on the cold call and accepts the warm one. Existing canary / prod / VPS installs were unaffected because their populated WindowsServer/ trees mean every boot's app_update is a verify-only no-download path that doesn't go through the failing licensing code. So this regression silently shipped to anyone trying a clean `docker compose up` while the live deployments kept working. Fix: retry the heredoc-based app_update up to 3 times, gated on server_files_present (the .exe being on disk after the run). SteamCMD resumes partial downloads across retries, so we don't pay bandwidth twice. Most fresh installs succeed on attempt #2. Switched from `+command` chain to stdin heredoc while we're here — heredoc reads more like the official Windrose dedicated-server FAQ examples that operators paste into interactive SteamCMD for debugging, and is functionally equivalent post-warm-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
seslly
added a commit
that referenced
this pull request
Apr 28, 2026
* entrypoint: retry SteamCMD app_update — fresh-install bootstrap was flaky User-reported fresh `docker compose up` failed with: ERROR! Failed to install app '4129620' (Missing configuration) ERROR: SteamCMD exited with 8 Root cause (verified by clean-container A/B): SteamCMD's anonymous app_update is flaky on the FIRST attempt within a container, then succeeds reliably on attempt #2 with no other change. Probably a Steam-side anonymous-license caching quirk that warms up after the first call. Fresh containers always hit attempt #1. Reproduced cleanly: cold attempt #1 FAILs, #2 OK + starts downloading, #3 resumes #2's partial download at ~50-65%. The depots are public anonymous regardless, the credentials are valid, the manifest is present — Steam just returns the cryptic "Missing configuration" on the cold call and accepts the warm one. Existing canary / prod / VPS installs were unaffected because their populated WindowsServer/ trees mean every boot's app_update is a verify-only no-download path that doesn't go through the failing licensing code. So this regression silently shipped to anyone trying a clean `docker compose up` while the live deployments kept working. Fix: retry the heredoc-based app_update up to 3 times, gated on server_files_present (the .exe being on disk after the run). SteamCMD resumes partial downloads across retries, so we don't pay bandwidth twice. Most fresh installs succeed on attempt #2. Switched from `+command` chain to stdin heredoc while we're here — heredoc reads more like the official Windrose dedicated-server FAQ examples that operators paste into interactive SteamCMD for debugging, and is functionally equivalent post-warm-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix ui container bind --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/tmpfirst.tarfile.add()over the liveR5/Savedtree races with RocksDB compactions; one reporter saw their world fail to load after a download and the server generate a fresh one. Nowshutil.copytreeto atempfile.TemporaryDirectoryfirst, tar the detached copy, stream that — download I/O never touches the live DB.reconcile_server_configwritesServerDescription.last-env-stamp.jsonafter each stamp and only re-stamps a key on later boots when its live value still matches the shadow. Diverged values (UI edit, ldb, manual jq) are loggedenv-mode: keeping operator-modified <key>and left alone. Fixes the "server name always resets to Windrose bare-linux after restart" report.R5/.maintenance-modemakes the entrypoint loop-sleep instead of launching Proton. Admin UI's new "Maintenance mode" card toggles via/api/maintenance({active, restart?}body); handler traps SIGTERM for clean shutdown. Works uniformly across Helm / compose / bare-Linux without needing special scale-down /systemctl stop/docker compose stopprivileges.README
Admin Console APItable gains the four new endpoints. Config-editor card blurb now mentions the shadow-stamp behavior so operators know UI edits survive env-mode restarts.Test plan
jqedit ofServerName→ pod restart → value preserved, entrypoint loggedINFO: env-mode: keeping operator-modified ServerNamePOST /api/maintenance {active: true}→ pod restart → entrypoint honored flag →POST {active: false, restart: true}→ game came back up cleanly/api/saves/downloadend-to-end on canary: produces identical tar contents, game unaffected (no patch to RocksDB)helm lint+helm template+py_compile+bash -n+node -call green🤖 Generated with Claude Code