feat: default persistent caching to verified lazy storage - #10754
dmadisetti wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
documentation
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
🟡 Changes recommended
Browser cryptography is not loaded, and asynchronous lazy writes introduce correctness and failure regressions for the new default.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Defaults persistent caching to signed lazy storage and strengthens verification behavior.
Changes:
- Switches all persistent-cache forms to lazy signed manifests.
- Preserves signing identity during corruption and concurrent creation.
- Updates dependencies, tests, and migration/browser documentation.
File summaries
| File | Description |
|---|---|
marimo/_save/save.py |
Changes the default cache method and API docs. |
marimo/_save/loaders/lazy.py |
Enforces verification without insecure fallback. |
marimo/_save/signing.py |
Publishes keys safely and preserves damaged files. |
pyproject.toml |
Promotes cryptography to a native dependency. |
docs/api/caching.md |
Documents formats, trust, and migration. |
docs/guides/exporting/webassembly_html.md |
Documents browser verification requirements. |
tests/_save/test_signed_defaults.py |
Tests defaults, concurrency, and restart verification. |
tests/_save/test_signing.py |
Tests damaged-key preservation. |
tests/_save/test_cache_versions.py |
Keeps legacy fixture explicitly pickle-based. |
tests/_save/store/test_store.py |
Keeps mock-store coverage on pickle. |
tests/_save/loaders/test_lazy_wasm.py |
Marks unsigned WASM fixture explicitly. |
tests/_save/loaders/test_lazy_signing.py |
Tests fail-closed verification policies. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Sign persistent cache manifests on native Python. | ||
| "cryptography>=42.0.0; sys_platform != 'emscripten'", |
| Cached exports require `cryptography` and a trusted signer in the browser to | ||
| restore signed entries. If either is unavailable, marimo recomputes the cells | ||
| instead of accepting unsigned or unverifiable data. A cell that requires a | ||
| native-only package cannot recompute in the browser. The previous automatic | ||
| same-origin verification bypass no longer applies. |
| verification != "off" | ||
| and self._can_verify() | ||
| and signer is not None | ||
| and signer.can_sign |
| name: str | Callable[..., Any] | None = None, | ||
| save_path: str | None = None, | ||
| method: LoaderKey = "pickle", | ||
| method: LoaderKey = "lazy", |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 12 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/_save/signing.py">
<violation number="1" location="marimo/_save/signing.py:492">
P2: Key publication now depends on `os.link` hard-link support. On filesystems that cannot create hard links (e.g. a FAT/exFAT or some network-mount state dir), `os.link` raises a non-`FileExistsError`, the `except Exception` swallows persisted-cache capability, and — with verification `on` no longer degrading — every persistent cache read misses and every write is skipped for the session. `os.replace` previously worked on those filesystems. Catch or fall back for the non-EEXIST case (e.g. retry with `os.replace`) so caching still works where hard links are unavailable.</violation>
<violation number="2" location="marimo/_save/signing.py:492">
P2: When a symlink is placed at `key_file` after the initial `exists()` check, `os.link` raises `FileExistsError` and the following `read_text()` follows it. The loader therefore adopts an attacker-selected private key, defeating the atomic non-replacement trust boundary; read the winner through a no-follow file descriptor and use the in-memory key for the publishing process.</violation>
</file>
<file name="pyproject.toml">
<violation number="1" location="pyproject.toml:14">
P2: `cryptography` is only consumed by the persistent-cache signing feature, which is coded to degrade gracefully without it (all uses are guarded by `DependencyManager.cryptography.has()` and imports are deferred; reads miss and writes are skipped while computation runs normally). Making it a hard base runtime dependency forces every user — including those who never use persistent caching — to install a native Rust-compiled package, enlarging installs and risking conflicts with pinned environments. Keep it out of the base install and wire it as a feature-scoped dependency (e.g., an extra such as `marimo[cache]` or back in `recommended`) so the default runtime still works through the existing fallback for users who install the signing capability explicitly.</violation>
</file>
<file name="docs/guides/exporting/webassembly_html.md">
<violation number="1" location="docs/guides/exporting/webassembly_html.md:80">
P2: This section makes a trusted signer in the browser a hard prerequisite for cached exports but never explains how a reader satisfies it, so the documented workflow can't be followed. The old same-origin bypass is gone, and `docs/api/caching.md` only explains trust via the writer's own user configuration, which doesn't exist for arbitrary users opening an exported HTML page. Explain how the export establishes the trusted signer (e.g. bundling the writer's public fingerprint into the export) or link to the configuration that applies in the browser; otherwise readers have no way to make cached exports restore entries.</violation>
<violation number="2" location="docs/guides/exporting/webassembly_html.md:84">
P3: The new paragraph is followed by two consecutive blank lines before 'With caching, you can publish WebAssembly notebooks...', while every other paragraph in this file uses a single blank line. Trim the extra blank line so the doc stays consistent and passes the repo's markdownlint MD012 rule.</violation>
</file>
<file name="docs/api/caching.md">
<violation number="1" location="docs/api/caching.md:107">
P3: The new trust section tells users to 'configure the writer's public fingerprint as a trusted signer in your user configuration' but never gives the configuration key, section, or value format. Readers cannot act on the instruction. Point to the exact key (e.g. `trusted_signers` with a `SHA256:<base64>` fingerprint under the cache/signing section) or link to the configuration reference so the sharing workflow is actionable.</violation>
</file>
<file name="tests/_save/test_signed_defaults.py">
<violation number="1" location="tests/_save/test_signed_defaults.py:107">
P3: The `os.replace` patch here never fires. `_get_machine_signer` publishes its key via `os.link(tmp, key_file)` (marimo/_save/signing.py:492) and does not call `os.replace` anywhere in the resolution path, so `publish` is never invoked with `original_replace`. Drop the `os.replace` patch (and the `original_replace` variable) so the test setup reflects the actual hard-link publish mechanism and doesn't imply `os.replace` is part of the identity-preserving logic.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| try: | ||
| os.unlink(tmp) | ||
| except OSError: | ||
| os.link(tmp, key_file) |
There was a problem hiding this comment.
P2: Key publication now depends on os.link hard-link support. On filesystems that cannot create hard links (e.g. a FAT/exFAT or some network-mount state dir), os.link raises a non-FileExistsError, the except Exception swallows persisted-cache capability, and — with verification on no longer degrading — every persistent cache read misses and every write is skipped for the session. os.replace previously worked on those filesystems. Catch or fall back for the non-EEXIST case (e.g. retry with os.replace) so caching still works where hard links are unavailable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_save/signing.py, line 492:
<comment>Key publication now depends on `os.link` hard-link support. On filesystems that cannot create hard links (e.g. a FAT/exFAT or some network-mount state dir), `os.link` raises a non-`FileExistsError`, the `except Exception` swallows persisted-cache capability, and — with verification `on` no longer degrading — every persistent cache read misses and every write is skipped for the session. `os.replace` previously worked on those filesystems. Catch or fall back for the non-EEXIST case (e.g. retry with `os.replace`) so caching still works where hard links are unavailable.</comment>
<file context>
@@ -485,34 +484,27 @@ def _get_machine_signer(
try:
- os.unlink(tmp)
- except OSError:
+ os.link(tmp, key_file)
+ except FileExistsError:
+ # The winner published its complete key before creating the
</file context>
| try: | ||
| os.unlink(tmp) | ||
| except OSError: | ||
| os.link(tmp, key_file) |
There was a problem hiding this comment.
P2: When a symlink is placed at key_file after the initial exists() check, os.link raises FileExistsError and the following read_text() follows it. The loader therefore adopts an attacker-selected private key, defeating the atomic non-replacement trust boundary; read the winner through a no-follow file descriptor and use the in-memory key for the publishing process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_save/signing.py, line 492:
<comment>When a symlink is placed at `key_file` after the initial `exists()` check, `os.link` raises `FileExistsError` and the following `read_text()` follows it. The loader therefore adopts an attacker-selected private key, defeating the atomic non-replacement trust boundary; read the winner through a no-follow file descriptor and use the in-memory key for the publishing process.</comment>
<file context>
@@ -485,34 +484,27 @@ def _get_machine_signer(
try:
- os.unlink(tmp)
- except OSError:
+ os.link(tmp, key_file)
+ except FileExistsError:
+ # The winner published its complete key before creating the
</file context>
| # Dependencies should have lower bounds, which should be as loose as possible. | ||
| dependencies = [ | ||
| # Sign persistent cache manifests on native Python. | ||
| "cryptography>=42.0.0; sys_platform != 'emscripten'", |
There was a problem hiding this comment.
P2: cryptography is only consumed by the persistent-cache signing feature, which is coded to degrade gracefully without it (all uses are guarded by DependencyManager.cryptography.has() and imports are deferred; reads miss and writes are skipped while computation runs normally). Making it a hard base runtime dependency forces every user — including those who never use persistent caching — to install a native Rust-compiled package, enlarging installs and risking conflicts with pinned environments. Keep it out of the base install and wire it as a feature-scoped dependency (e.g., an extra such as marimo[cache] or back in recommended) so the default runtime still works through the existing fallback for users who install the signing capability explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 14:
<comment>`cryptography` is only consumed by the persistent-cache signing feature, which is coded to degrade gracefully without it (all uses are guarded by `DependencyManager.cryptography.has()` and imports are deferred; reads miss and writes are skipped while computation runs normally). Making it a hard base runtime dependency forces every user — including those who never use persistent caching — to install a native Rust-compiled package, enlarging installs and risking conflicts with pinned environments. Keep it out of the base install and wire it as a feature-scoped dependency (e.g., an extra such as `marimo[cache]` or back in `recommended`) so the default runtime still works through the existing fallback for users who install the signing capability explicitly.</comment>
<file context>
@@ -10,6 +10,8 @@ description = "A library for making reactive notebooks and apps"
# Dependencies should have lower bounds, which should be as loose as possible.
dependencies = [
+ # Sign persistent cache manifests on native Python.
+ "cryptography>=42.0.0; sys_platform != 'emscripten'",
# For maintainable cli
"click>=8.0,<9; sys_platform != 'emscripten'",
</file context>
|
|
||
| ## Exporting with cached execution { #exporting-with-cached-execution } | ||
|
|
||
| Cached exports require `cryptography` and a trusted signer in the browser to |
There was a problem hiding this comment.
P2: This section makes a trusted signer in the browser a hard prerequisite for cached exports but never explains how a reader satisfies it, so the documented workflow can't be followed. The old same-origin bypass is gone, and docs/api/caching.md only explains trust via the writer's own user configuration, which doesn't exist for arbitrary users opening an exported HTML page. Explain how the export establishes the trusted signer (e.g. bundling the writer's public fingerprint into the export) or link to the configuration that applies in the browser; otherwise readers have no way to make cached exports restore entries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/guides/exporting/webassembly_html.md, line 80:
<comment>This section makes a trusted signer in the browser a hard prerequisite for cached exports but never explains how a reader satisfies it, so the documented workflow can't be followed. The old same-origin bypass is gone, and `docs/api/caching.md` only explains trust via the writer's own user configuration, which doesn't exist for arbitrary users opening an exported HTML page. Explain how the export establishes the trusted signer (e.g. bundling the writer's public fingerprint into the export) or link to the configuration that applies in the browser; otherwise readers have no way to make cached exports restore entries.</comment>
<file context>
@@ -77,6 +77,13 @@ incompatibilities early:
## Exporting with cached execution { #exporting-with-cached-execution }
+Cached exports require `cryptography` and a trusted signer in the browser to
+restore signed entries. If either is unavailable, marimo recomputes the cells
+instead of accepting unsigned or unverifiable data. A cell that requires a
</file context>
| restore signed entries. If either is unavailable, marimo recomputes the cells | ||
| instead of accepting unsigned or unverifiable data. A cell that requires a | ||
| native-only package cannot recompute in the browser. The previous automatic | ||
| same-origin verification bypass no longer applies. |
There was a problem hiding this comment.
P3: The new paragraph is followed by two consecutive blank lines before 'With caching, you can publish WebAssembly notebooks...', while every other paragraph in this file uses a single blank line. Trim the extra blank line so the doc stays consistent and passes the repo's markdownlint MD012 rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/guides/exporting/webassembly_html.md, line 84:
<comment>The new paragraph is followed by two consecutive blank lines before 'With caching, you can publish WebAssembly notebooks...', while every other paragraph in this file uses a single blank line. Trim the extra blank line so the doc stays consistent and passes the repo's markdownlint MD012 rule.</comment>
<file context>
@@ -77,6 +77,13 @@ incompatibilities early:
+restore signed entries. If either is unavailable, marimo recomputes the cells
+instead of accepting unsigned or unverifiable data. A cell that requires a
+native-only package cannot recompute in the browser. The previous automatic
+same-origin verification bypass no longer applies.
+
+
</file context>
| is unavailable, reads miss and writes are skipped, including in WebAssembly. | ||
| Missing signing capability does not prevent the computation from running. | ||
|
|
||
| To share caches, configure the writer's public fingerprint as a trusted signer |
There was a problem hiding this comment.
P3: The new trust section tells users to 'configure the writer's public fingerprint as a trusted signer in your user configuration' but never gives the configuration key, section, or value format. Readers cannot act on the instruction. Point to the exact key (e.g. trusted_signers with a SHA256:<base64> fingerprint under the cache/signing section) or link to the configuration reference so the sharing workflow is actionable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/api/caching.md, line 107:
<comment>The new trust section tells users to 'configure the writer's public fingerprint as a trusted signer in your user configuration' but never gives the configuration key, section, or value format. Readers cannot act on the instruction. Point to the exact key (e.g. `trusted_signers` with a `SHA256:<base64>` fingerprint under the cache/signing section) or link to the configuration reference so the sharing workflow is actionable.</comment>
<file context>
@@ -91,6 +91,34 @@ letting you pick up where you left off.
+is unavailable, reads miss and writes are skipped, including in WebAssembly.
+Missing signing capability does not prevent the computation from running.
+
+To share caches, configure the writer's public fingerprint as a trusted signer
+in your user configuration. Keep the writer's private key private. Trust a
+signer only if you trust the code in its cached payloads: signatures do not
</file context>
|
|
||
| with ( | ||
| patch.object(signing, "generate_keypair", generate), | ||
| patch.object( |
There was a problem hiding this comment.
P3: The os.replace patch here never fires. _get_machine_signer publishes its key via os.link(tmp, key_file) (marimo/_save/signing.py:492) and does not call os.replace anywhere in the resolution path, so publish is never invoked with original_replace. Drop the os.replace patch (and the original_replace variable) so the test setup reflects the actual hard-link publish mechanism and doesn't imply os.replace is part of the identity-preserving logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/_save/test_signed_defaults.py, line 107:
<comment>The `os.replace` patch here never fires. `_get_machine_signer` publishes its key via `os.link(tmp, key_file)` (marimo/_save/signing.py:492) and does not call `os.replace` anywhere in the resolution path, so `publish` is never invoked with `original_replace`. Drop the `os.replace` patch (and the `original_replace` variable) so the test setup reflects the actual hard-link publish mechanism and doesn't imply `os.replace` is part of the identity-preserving logic.</comment>
<file context>
@@ -0,0 +1,162 @@
+
+ with (
+ patch.object(signing, "generate_keypair", generate),
+ patch.object(
+ os, "replace", lambda *a, **kw: publish(original_replace, *a, **kw)
+ ),
</file context>
This pull request was authored by a coding agent.
Summary
Stacked on #10753; review this PR against
dm/cache-key-correctness.mo.persistent_cachecurrently defaults to unsigned pickle, while automatic cell caching uses the lazy manifest/blob format. This change defaults persistent caching to lazy, signed storage at the same release boundary as the cache-key invalidation in #10753.method="lazy"; retain explicit pickle and JSON methods.cryptographya native runtime dependency. Verificationonnever automatically becomesoff: without verification capability, reads miss and writes are skipped while the computation runs normally. Explicitoffandstrictremain available.Compatibility and review points
This intentionally removes the local-file and same-origin WASM verification bypasses. Browser caches require cryptography and a configured trusted signer; otherwise cells recompute. Native-only computations cannot recompute in the browser, so existing cached exports that relied on the bypass need explicit trust/verification configuration before this merges.
Automatic key publication requires hard-link support in the user state directory. If it fails, verified persistence is unavailable and computations still run. The signing algorithm and manifest signature format are unchanged. No browser/GPU or performance benchmark was run.
Validation
make checkpasses with Nix-provided make, pnpm, and pixi: frontend lint/type checking, typos, copyright, Ruff, all 787 Python modules, and lock validation.Pre-review checklist
Merge checklist