fix(toolkit): write wallet-cache entries at the umask default, and stop the GC sweep deleting what it cannot read - #2081
Conversation
…op the GC sweep deleting what it cannot read Both writers in the file cache backend staged through `NamedTempFile::new_in`, which hard-codes mode 0600, and `persist()` uses `rename(2)` — so 0600 landed on the finished cache file. A umask can only clear bits, never add them, so no umask setting could widen it. A `--ledger-state-db` shared between users is the normal case on a perf box: CI jobs run as one user, interactive sessions as another, both in a common group over a setgid directory. Every user but the writer therefore saw each seed as a cache miss — and one missing entry forces `build_fork_aware_context_cached` to replay from genesis for the whole seed set, ~90 minutes on a 31k-block chain. Two further defects turned that from slow into destructive: - `get_all_cached_wallet_heights` exists to collect snapshot GC references, but it scanned the *entire* wallets directory and deleted any file whose header it could not parse — and `read_wallet_height` was `.ok()?` throughout, so it could not tell "unreadable by this user" from "corrupt". That scan runs on every cache save by every command, so two users sharing a directory destroyed each other's entries on every save, as would two toolkit builds with different `WALLET_CACHE_FORMAT_VERSION` values. - `get_wallet_states` turned any io error into an unlogged miss, so the resulting replay had nothing in the log to explain itself. - New `staging_file` helper for both writers: asks for 0666 and lets the process umask narrow it (0002 -> 0664, the usual 0022 -> 0644, 0077 -> 0600 for a deliberately private cache). Directories were already left to the umask by `fs::create_dir_all`, so one umask now governs both. - `read_wallet_height` returns `io::Result<Option<u64>>`: `Err` for "could not read", `Ok(None)` for "not this format version". - The GC scan is read-only. An entry it cannot account for is not counted as a snapshot reference, and is left alone. Eviction of genuinely corrupt entries stays in `get_wallet_states`, scoped to the seeds the caller asked for, where the format-versioned cache key means an undecodable body really is corruption. - `write_wallet_if_newer` logs, then replaces, an entry whose height it cannot read — which is how a directory of 0600 entries heals. Tests pin the written mode against `open(O_CREAT, 0o666)` under the live umask rather than a literal, so the intent holds whatever umask CI uses, and cover the sweep sparing both unreadable and foreign-format entries, an unreadable entry becoming usable again once readable, and permissions healing on replace. Assisted-by: Claude:claude-opus-5 Signed-off-by: chrispalaskas <chris.palaskas@gmail.com>
chrispalaskas
added
skip-changes-check-issue
bot:ai-assisted
Assisted-by: Claude:claude-opus-5 Signed-off-by: chrispalaskas <chris.palaskas@gmail.com>
![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: ff60193f76
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Ok(None) => log::debug!( | ||
| "Ignoring wallet cache file {name}: not this cache format version" | ||
| ), | ||
| Err(e) if e.kind() == io::ErrorKind::NotFound => {}, | ||
| Err(e) => log::warn!("Ignoring unreadable wallet cache file {name}: {e}"), |
There was a problem hiding this comment.
When a wallet entry is unreadable or from another cache format, these branches omit its height from the returned reference set. try_save_cache_v2 passes that set directly to gc_ledger_snapshots (util/toolkit/src/tx_generator/builder/mod.rs:1552-1558), so another user or toolkit version can still delete the wallet's required ledger snapshot once it is outside the grace period and newest-two floor. The wallet file survives, but it becomes unusable even after permissions are restored; GC should be skipped or made conservative whenever any wallet height cannot be accounted for.
Useful? React with 👍 / 👎.
|
1 participant
Overview
A
--ledger-state-dbshared between users silently behaved as if it were empty for everyone but the user who wrote it, and each save actively deleted the other user's entries.Both writers in the file cache backend staged through
NamedTempFile::new_in, which hard-codes mode 0600, andpersist()usesrename(2)— so 0600 landed on the finished cache file. A umask can only clear bits, never add them, so no umask setting could widen it.Sharing that directory is the normal case on a perf box: CI jobs run as one user, interactive sessions as another, both in a common group over a setgid directory. Every user but the writer saw each seed as a cache miss — and one missing entry forces
build_fork_aware_context_cachedto replay from genesis for the whole seed set (builder/mod.rs:1337), which is ~90 minutes on a 31k-block chain. Two overnight load-generation rounds onhel1-perf-01produced zero transactions this way.Two further defects turned that from slow into destructive:
get_all_cached_wallet_heightsexists to collect snapshot-GC references, but it scanned the entire wallets directory and deleted any file whose 9-byte header it could not parse — andread_wallet_heightwas.ok()?throughout, so it could not tell "unreadable by this user" from "corrupt". That scan runs on every cache save by every command, so two users sharing a directory destroyed each other's entries on every save. So would two toolkit builds with differentWALLET_CACHE_FORMAT_VERSIONvalues.get_wallet_statesturned any io error into an unlogged miss, so the resulting hour-and-a-half replay had nothing in the log to explain itself.Changes
staging_filehelper used by both writers: asks for0666and lets the process umask narrow it —0002→ 0664, the usual0022→ 0644,0077→ 0600 for a deliberately private cache. Directories were already left to the umask byfs::create_dir_all, so one umask now governs both halves rather than the files being immovable.#[cfg(unix)]-gated.read_wallet_heightreturnsio::Result<Option<u64>>:Errfor "could not read",Ok(None)for "not this format version".get_wallet_states, scoped to the seeds the caller asked for, where the format-versioned cache key means an undecodable body really is corruption.write_wallet_if_newerlogs, then replaces, an entry whose height it cannot read — which is how a directory full of 0600 entries heals.get_wallet_stateslogs an unreadable entry instead of silently reporting a miss.Deliberately no policy imposed: the toolkit asks for the permissive mode and the deployment's umask decides. A private cache under
umask 0077still comes out 0600.🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCOExisting on-disk caches need no migration: entries already written 0600 keep working for their owner, and are replaced at the new mode the next time their height advances. The companion infra PR reconciles the modes of what is already on disk.
🧪 Testing Evidence
cargo test -p midnight-node-toolkit --lib --test cached_context— 139 passed, 0 failed plus thecached_contextintegration test, which drivesbuild_fork_aware_context_cachedagainst the file backend and asserts it matches an uncached build:cargo clippy -p midnight-node-toolkit --all-targetsclean;cargo fmtapplied.The four new tests, and one renamed:
cache_files_are_written_at_the_umask_default_not_owner_only— pins the mode of both a wallet entry and a ledger snapshot against whatopen(O_CREAT, 0o666)yields under the live umask, rather than against a literal, so it asserts the actual intent ("ask for 0666, let the umask decide") whatever umask CI runs with.sweep_ignores_unreadable_entry_instead_of_deleting_it— a valid entry chmod'd000: not counted as a reference, not deleted, reads back as a miss, and is usable again once readable. This is the regression that matters most.sweep_ignores_foreign_format_entry_instead_of_deleting_it— a v1-layout entry (bare 8-byte LE height) survives the scan.unreadable_entry_is_replaced_and_permissions_heal— replacing an entry whose height cannot be read writes the replacement at the umask default.corrupted_wallet_file_is_deleted→corrupted_wallet_file_is_evicted_on_read: same guarantee, moved to the path that legitimately owns eviction. Deliberate behaviour change — the sweep no longer deletes.Also verified before the fix, with a throwaway probe against a temp dir, that a small-seed-set save does not evict a large warm cache (1000/1000 entries and the referenced snapshot survive) — the eviction was entirely the unreadable-file path, not
set_wallet_statesor snapshot GC. Not applied tohel1-perf-01yet; that wants the companion infra change first, since a fresh toolkit writing 0664 into directories still at 0755 only half-works.🔱 Fork Strategy
Links
Companion infra change (runner
UMask=0002+ mode reconcile on the shared perf directories): shieldedtech/shielded-iac#2194 — the two are complementary, and this one is the half a umask cannot do.