feat: marimo cache clean - #10839
Draft
dmadisetti wants to merge 1 commit into
Draft
feat: marimo cache clean#10839dmadisetti wants to merge 1 commit into
dmadisetti wants to merge 1 commit into
Conversation
Adds marimo cache clean [PATH] [NAME]..., with a confirmation prompt and -y/--yes/--force (also honoring the global -y). A directory PATH wipes whole block directories, blobs included, never touching root dot-files. A notebook PATH deletes exactly the entries its manifest lists, jsonl marker first, blob directory only when no surviving jsonl still attests it, then discards those records and rewrites the manifest atomically. Manifest-sourced names are validated as single restricted path components before any deletion, so a hostile manifest cannot reach outside the cache directory. Also fixes BasePersistenceLoader.clear() orphaning lazily written blob directories: enumeration now asks the store for its clearable root instead of requiring a bare FileStore, so the production LazyLoader/LazyStore wiring clears and reports blob bytes correctly.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
9 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="marimo/_cli/cache/commands.py">
<violation number="1" location="marimo/_cli/cache/commands.py:265">
P1: Notebook-path cleanup currently crashes before reading the manifest because `manifest_name()` references the undefined `MANIFEST_PREFIX`. Define that prefix in `marimo/_save/manifest.py` before wiring this command to it.</violation>
<violation number="2" location="marimo/_cli/cache/commands.py:271">
P2: When the manifest path cannot be read, `load_manifest` raises `OSError`, which this handler does not convert into a CLI error. Catch `OSError` alongside `ManifestError` so cache cleanup fails with the command's normal error message.</violation>
<violation number="3" location="marimo/_cli/cache/commands.py:295">
P2: When the cache changes between confirmation and deletion, the command can delete or retain different files from the confirmed plan while still rewriting the manifest. Reuse a single snapshot/plan for confirmation and execution, or revalidate the plan before deleting and updating the manifest.</violation>
</file>
<file name="marimo/_save/stores/file.py">
<violation number="1" location="marimo/_save/stores/file.py:124">
P1: When `clear()` receives a traversal key, this branch recursively deletes directories outside `save_path` because it performs no containment check. Validate every resolved deletion path beneath the store root before `rmtree` (and before the existing `unlink` path).</violation>
</file>
<file name="marimo/_save/loaders/loader.py">
<violation number="1" location="marimo/_save/loaders/loader.py:324">
P1: When different persistent-cache methods share a name, clearing one loader now removes the other loader's blob directories. Keep directory deletion scoped to blob directories owned by this loader; otherwise the surviving lazy manifest points to deleted blobs and the other cache is corrupted.</violation>
<violation number="2" location="marimo/_save/loaders/loader.py:324">
P3: Clearing a lazy cache now removes blob directories by calling `store.clear("block/hash")`, but `LazyStore.clear` only discards that exact key from `_written_keys`/`_touched_keys`. Blobs were tracked under individual paths (`block/hash/x.pickle`), so after `loader.clear()` those stale keys remain and are still returned by `export_keys()`. If the same session later dumps an export manifest (`dump_cache_manifests`), it will list blob files that were just deleted, producing an incomplete bundle. This stale tracking is newly introduced by the blob-directory clearing path.</violation>
</file>
<file name="tests/_save/test_cache_dirs.py">
<violation number="1" location="tests/_save/test_cache_dirs.py:621">
P3: In `test_delete_entries_refuses_a_name_that_leaves_the_cache`, the comment on the `("train", "C_..")` entry misdescribes both the real danger and the rejection path. `_ENTRY_KEY = re.compile(r"[A-Za-z0-9_-]+")` rejects any key containing `.`, so `C_..` never reaches hash derivation; and even if it did, `_entry_hash("C_..")` splits on the first `.` into stem `"C_"`, which is not the block's own parent. The comment implies traversal can be attempted via a key whose hash is a parent directory, which is not what happens, and misleads future maintainers about which property the guard actually enforces.</violation>
</file>
<file name="marimo/_save/cache_dirs.py">
<violation number="1" location="marimo/_save/cache_dirs.py:318">
P2: When an entry or its blobs cannot be removed, this line still reports one freed entry and the CLI forgets that manifest record. Count entries only after confirming deletion succeeded, and preserve records for paths that remain.</violation>
<violation number="2" location="marimo/_save/cache_dirs.py:332">
P1: A hostile manifest can pass a non-cache key such as `foo`, causing `_delete_block_entries` to delete the arbitrary `cache_dir/<block>/foo` blob directory. Validate the complete cache-key grammar, including its type prefix and digest shape, before deriving `entry_hash`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ) | ||
| from marimo._save.stores.file import FileStore | ||
|
|
||
| key = manifest_name(notebook) |
Contributor
There was a problem hiding this comment.
P1: Notebook-path cleanup currently crashes before reading the manifest because manifest_name() references the undefined MANIFEST_PREFIX. Define that prefix in marimo/_save/manifest.py before wiring this command to it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_cli/cache/commands.py, line 265:
<comment>Notebook-path cleanup currently crashes before reading the manifest because `manifest_name()` references the undefined `MANIFEST_PREFIX`. Define that prefix in `marimo/_save/manifest.py` before wiring this command to it.</comment>
<file context>
@@ -128,27 +156,182 @@ def cache_dir(path: Path, recursive: bool) -> None:
+ )
+ from marimo._save.stores.file import FileStore
+
+ key = manifest_name(notebook)
+ store = FileStore(save_path=str(cache_dir))
+ manifest = None
</file context>
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| # A value stored in parts is a directory of them, which unlinking | ||
| # cannot remove and whose reported size holds no bytes of its own. | ||
| if path.is_dir() and not path.is_symlink(): |
Contributor
There was a problem hiding this comment.
P1: When clear() receives a traversal key, this branch recursively deletes directories outside save_path because it performs no containment check. Validate every resolved deletion path beneath the store root before rmtree (and before the existing unlink path).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_save/stores/file.py, line 124:
<comment>When `clear()` receives a traversal key, this branch recursively deletes directories outside `save_path` because it performs no containment check. Validate every resolved deletion path beneath the store root before `rmtree` (and before the existing `unlink` path).</comment>
<file context>
@@ -115,6 +119,11 @@ def hit(self, key: str) -> bool:
path.parent.mkdir(parents=True, exist_ok=True)
+ # A value stored in parts is a directory of them, which unlinking
+ # cannot remove and whose reported size holds no bytes of its own.
+ if path.is_dir() and not path.is_symlink():
+ shutil.rmtree(path, ignore_errors=True)
+ return not path.exists()
</file context>
| # beside its entry. Removing the entry alone leaves those bytes | ||
| # behind with nothing left that can read them. | ||
| paths.extend( | ||
| child for child in _subdirectories(block) if child not in paths |
Contributor
There was a problem hiding this comment.
P1: When different persistent-cache methods share a name, clearing one loader now removes the other loader's blob directories. Keep directory deletion scoped to blob directories owned by this loader; otherwise the surviving lazy manifest points to deleted blobs and the other cache is corrupted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_save/loaders/loader.py, line 324:
<comment>When different persistent-cache methods share a name, clearing one loader now removes the other loader's blob directories. Keep directory deletion scoped to blob directories owned by this loader; otherwise the surviving lazy manifest points to deleted blobs and the other cache is corrupted.</comment>
<file context>
@@ -306,9 +314,16 @@ def _clearable_paths(self, root: Path) -> list[Path]:
+ # beside its entry. Removing the entry alone leaves those bytes
+ # behind with nothing left that can read them.
+ paths.extend(
+ child for child in _subdirectories(block) if child not in paths
+ )
+ return paths
</file context>
| return ( | ||
| bool(block) | ||
| and block == block_dir_name(block) | ||
| and _ENTRY_KEY.fullmatch(key) is not None |
Contributor
There was a problem hiding this comment.
P1: A hostile manifest can pass a non-cache key such as foo, causing _delete_block_entries to delete the arbitrary cache_dir/<block>/foo blob directory. Validate the complete cache-key grammar, including its type prefix and digest shape, before deriving entry_hash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_save/cache_dirs.py, line 332:
<comment>A hostile manifest can pass a non-cache key such as `foo`, causing `_delete_block_entries` to delete the arbitrary `cache_dir/<block>/foo` blob directory. Validate the complete cache-key grammar, including its type prefix and digest shape, before deriving `entry_hash`.</comment>
<file context>
@@ -164,6 +268,115 @@ def entry_bytes(entry: Path) -> int:
+ return (
+ bool(block)
+ and block == block_dir_name(block)
+ and _ENTRY_KEY.fullmatch(key) is not None
+ )
+
</file context>
Suggested change
| and _ENTRY_KEY.fullmatch(key) is not None | |
| and re.fullmatch(r"[CDEPUX]_[A-Za-z0-9_-]{43}", key) is not None |
| if cache_dir.is_dir(): | ||
| try: | ||
| manifest = load_manifest(store, key) | ||
| except ManifestError as e: |
Contributor
There was a problem hiding this comment.
P2: When the manifest path cannot be read, load_manifest raises OSError, which this handler does not convert into a CLI error. Catch OSError alongside ManifestError so cache cleanup fails with the command's normal error message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_cli/cache/commands.py, line 271:
<comment>When the manifest path cannot be read, `load_manifest` raises `OSError`, which this handler does not convert into a CLI error. Catch `OSError` alongside `ManifestError` so cache cleanup fails with the command's normal error message.</comment>
<file context>
@@ -128,27 +156,182 @@ def cache_dir(path: Path, recursive: bool) -> None:
+ if cache_dir.is_dir():
+ try:
+ manifest = load_manifest(store, key)
+ except ManifestError as e:
+ raise MarimoCLIError(str(e)) from e
+ if manifest is None:
</file context>
Suggested change
| except ManifestError as e: | |
| except (ManifestError, OSError) as e: |
| _unlink(file) for file in _marker_first(entry_files) | ||
| ) | ||
| bytes_freed += _remove_tree(blobs) if blobs is not None else 0 | ||
| freed += CacheDirStats(total_bytes=bytes_freed, entries=1) |
Contributor
There was a problem hiding this comment.
P2: When an entry or its blobs cannot be removed, this line still reports one freed entry and the CLI forgets that manifest record. Count entries only after confirming deletion succeeded, and preserve records for paths that remain.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_save/cache_dirs.py, line 318:
<comment>When an entry or its blobs cannot be removed, this line still reports one freed entry and the CLI forgets that manifest record. Count entries only after confirming deletion succeeded, and preserve records for paths that remain.</comment>
<file context>
@@ -164,6 +268,115 @@ def entry_bytes(entry: Path) -> int:
+ _unlink(file) for file in _marker_first(entry_files)
+ )
+ bytes_freed += _remove_tree(blobs) if blobs is not None else 0
+ freed += CacheDirStats(total_bytes=bytes_freed, entries=1)
+ return freed
+
</file context>
| if deleting and not _confirm(planned, yes=yes): | ||
| return | ||
|
|
||
| freed = ( |
Contributor
There was a problem hiding this comment.
P2: When the cache changes between confirmation and deletion, the command can delete or retain different files from the confirmed plan while still rewriting the manifest. Reuse a single snapshot/plan for confirmation and execution, or revalidate the plan before deleting and updating the manifest.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_cli/cache/commands.py, line 295:
<comment>When the cache changes between confirmation and deletion, the command can delete or retain different files from the confirmed plan while still rewriting the manifest. Reuse a single snapshot/plan for confirmation and execution, or revalidate the plan before deleting and updating the manifest.</comment>
<file context>
@@ -128,27 +156,182 @@ def cache_dir(path: Path, recursive: bool) -> None:
+ if deleting and not _confirm(planned, yes=yes):
+ return
+
+ freed = (
+ delete_cache_entries(cache_dir, entries)
+ if deleting
</file context>
| ("../..", "model"), | ||
| ("train", ".."), | ||
| ("train", "../../model"), | ||
| # A key whose hash reads as the block's own parent. |
Contributor
There was a problem hiding this comment.
P3: In test_delete_entries_refuses_a_name_that_leaves_the_cache, the comment on the ("train", "C_..") entry misdescribes both the real danger and the rejection path. _ENTRY_KEY = re.compile(r"[A-Za-z0-9_-]+") rejects any key containing ., so C_.. never reaches hash derivation; and even if it did, _entry_hash("C_..") splits on the first . into stem "C_", which is not the block's own parent. The comment implies traversal can be attempted via a key whose hash is a parent directory, which is not what happens, and misleads future maintainers about which property the guard actually enforces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/_save/test_cache_dirs.py, line 621:
<comment>In `test_delete_entries_refuses_a_name_that_leaves_the_cache`, the comment on the `("train", "C_..")` entry misdescribes both the real danger and the rejection path. `_ENTRY_KEY = re.compile(r"[A-Za-z0-9_-]+")` rejects any key containing `.`, so `C_..` never reaches hash derivation; and even if it did, `_entry_hash("C_..")` splits on the first `.` into stem `"C_"`, which is not the block's own parent. The comment implies traversal can be attempted via a key whose hash is a parent directory, which is not what happens, and misleads future maintainers about which property the guard actually enforces.</comment>
<file context>
@@ -437,3 +449,262 @@ def test_entry_bytes(tmp_path: Path) -> None:
+ ("../..", "model"),
+ ("train", ".."),
+ ("train", "../../model"),
+ # A key whose hash reads as the block's own parent.
+ ("train", "C_.."),
+ ],
</file context>
Suggested change
| # A key whose hash reads as the block's own parent. | |
| # A key with a separator fails the allowed-key charset, | |
| # which is what keeps an entry from escaping its block. |
| # beside its entry. Removing the entry alone leaves those bytes | ||
| # behind with nothing left that can read them. | ||
| paths.extend( | ||
| child for child in _subdirectories(block) if child not in paths |
Contributor
There was a problem hiding this comment.
P3: Clearing a lazy cache now removes blob directories by calling store.clear("block/hash"), but LazyStore.clear only discards that exact key from _written_keys/_touched_keys. Blobs were tracked under individual paths (block/hash/x.pickle), so after loader.clear() those stale keys remain and are still returned by export_keys(). If the same session later dumps an export manifest (dump_cache_manifests), it will list blob files that were just deleted, producing an incomplete bundle. This stale tracking is newly introduced by the blob-directory clearing path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_save/loaders/loader.py, line 324:
<comment>Clearing a lazy cache now removes blob directories by calling `store.clear("block/hash")`, but `LazyStore.clear` only discards that exact key from `_written_keys`/`_touched_keys`. Blobs were tracked under individual paths (`block/hash/x.pickle`), so after `loader.clear()` those stale keys remain and are still returned by `export_keys()`. If the same session later dumps an export manifest (`dump_cache_manifests`), it will list blob files that were just deleted, producing an incomplete bundle. This stale tracking is newly introduced by the blob-directory clearing path.</comment>
<file context>
@@ -306,9 +314,16 @@ def _clearable_paths(self, root: Path) -> list[Path]:
+ # beside its entry. Removing the entry alone leaves those bytes
+ # behind with nothing left that can read them.
+ paths.extend(
+ child for child in _subdirectories(block) if child not in paths
+ )
+ return paths
</file context>
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
1 participant
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.
Stacked on #10838; review against
dm/cache-manifest.📝 Summary
Adds marimo cache clean [PATH] [NAME]..., with a confirmation prompt
and -y/--yes/--force (also honoring the global -y). A directory PATH
wipes whole block directories, blobs included, never touching root
dot-files. A notebook PATH deletes exactly the entries its manifest
lists, jsonl marker first, blob directory only when no surviving jsonl
still attests it, then discards those records and rewrites the manifest
atomically. Manifest-sourced names are validated as single restricted
path components before any deletion, so a hostile manifest cannot reach
outside the cache directory.
Also fixes BasePersistenceLoader.clear() orphaning lazily written blob
directories: enumeration now asks the store for its clearable root
instead of requiring a bare FileStore, so the production
LazyLoader/LazyStore wiring clears and reports blob bytes correctly.