Build: harden metadata storage (guarded load, atomic save) #56
Description
Question
Build: harden MetadataStorage (devlaunch/worktree/storage.py) so a corrupt or concurrently-written metadata.json can't crash the CLI or lose state.
_load()(storage.py:30-47) has no guard aroundjson.load— a corrupt/truncated file raises during construction and takes down everydlcommand. Guard it (recover or quarantine + start fresh), matching the patternread_completion_cachealready uses (dl.py:72-76).save()(storage.py:56-57) is a plain open+write — no temp-file+rename. An interrupted write corrupts state, and the background--update-cacheprocess runs concurrently with the foreground one. Make it atomic like the completion-cache writes (dl.py:84-89).add_worktree(storage.py:81-92) saves twice (once viaadd_repository, once itself) — collapse to one.
Whether metadata.json also gains a schema-version field is fog on the map — surface it in the resolution if this build makes it specifiable.
Activity
blooop commentedon Aug 7, 2026
Explicitly deferred to the Rust rewrite effort by #53 (go). The map's destination admits "landed as merged fix PRs or explicitly deferred" — this is the second outcome, not an abandonment.
Why deferred rather than built: this ticket's value is the design — guard the load, make the save atomic, collapse the double-save — and that design carries to
devlaunch-coreintact. Most of it also dissolves in Rust: serde-derive replaces the hand-rolledto_dict/from_dict, aResultat the parse boundary makes the unguardedjson.load(storage.py:30-47) unrepresentable rather than merely guarded, and atomic save isstd::fs::rename. Building it in Python first means implementing it twice.Carried forward as a requirement on
devlaunch-core, along with themetadata.jsonschema-version question that was fog on the map — the format is shared between the Python and Rust implementations across the parallel-migration period, so versioning is now a core-design question rather than a Python build.blooop commentedon Aug 7, 2026
Deferral reversed — lands in Python, in the final Python release. Same correction as #60.
One caveat that survives: the guarded-load and atomic-save work genuinely does get simpler in Rust (serde at the parse boundary,
std::fs::rename), so this is the one reopened ticket where some rework is real. It lands anyway because a corruptmetadata.jsontaking down everydlcommand (storage.py:30-47) is a live crash for current users, and the final Python release is the last chance to fix it for them.The
metadata.jsonschema-version question stays fog — but it is now sharper than before: the format is shared across the parallel-migration period, read by both the Python and Rust implementations, so whatever this build decides about versioning binds the core too. Surface it in the resolution.blooop commentedon Aug 7, 2026
Answer
Landed in #67. All three items done, plus eight holes adversarial review found in the first cut — each now pinned by a test verified red-before-green.
The three named:
_loadis total (unreadable or non-object file quarantined tometadata.json.corrupt, a single malformed entry skipped rather than costing the file);save()is a fsyncedmkstemp+ rename that does not swallow write failures;add_worktreeandremove_worktreeeach write once instead of twice.What review changed
Two were outright bugs in the fix itself. Catching
json.JSONDecodeErrorinstead ofValueErrorletUnicodeDecodeErrorfrom non-UTF-8 bytes escape the guard — the exact crash this ticket exists to prevent. Andtemp_path.replace()was a regression vs HEAD: it replaced a symlinkedmetadata.jsoninstead of writing through it, silently breaking the link and every later write. Also fixed: a widened file mode (0600->0664), aversion: 1.0header quarantining a perfectly loadable file, and a pid-based temp name that two instances in one process shared.The structural one: file-level damage was quarantined and recoverable, but a skipped entry was deleted permanently by the next write, with no backup. There is now one rule — any load that drops information copies the original to
metadata.json.bakat load time, before a write can happen, and names the path on stderr.Schema version — the fog is resolved, and implemented
SCHEMA_VERSION = 1is written as a top-level key; absent means legacy, read as 1; a non-integral header never costs the entries, which do not depend on it.schema_versionis exposed so #65 gets a deterministic, idempotent migration trigger instead of sniffing directory names — which #55 noted had a known false positive (a branch literally namedfoo-bexoza).One thing pulled forward deliberately
from_dictdidcls(**data), so a field added by a newer build raisedTypeError. Every entry has the same shape, so one new field made every entry unreadable and the old build would rewrite the file with an empty worktree list. That is a live risk precisely because #62 makes this the last Python release — this is the build left behind while the schema moves on.from_dictnow ignores fields it does not declare, warns naming them, and preserves the original.Correcting the ticket's premise
The ticket said "the background
--update-cacheprocess runs concurrently with the foreground one". It does, but it is not a writer of this file:update_completion_cache(dl.py:170) only reads devpod and git state and writes the completion caches. The comment insave()claiming concurrency safety was removed rather than reworded.Fog this leaves
save()serializes the whole document, so two concurrent foregrounddlprocesses can still lose an update — atomic rename prevents corruption, not read-modify-write races. Needs a lock or a re-read-before-write; out of this slice.Also noted:
write_completion_cache(dl.py:84) still uses a shared temp path and swallowsOSError, the same two flaws fixed here. Not touched, since it is a genuinely disposable cache.Rename existing clone dirs onto the new id scheme