Skip to content

fix(remote): refuse Path values at the SshCache boundary - #828

Merged
pmrv merged 4 commits into
temppathfrom
claude/temppath-comment-r1zhhx
Aug 6, 2026
Merged

fix(remote): refuse Path values at the SshCache boundary#828
pmrv merged 4 commits into
temppathfrom
claude/temppath-comment-r1zhhx

Conversation

@pmrv

@pmrv pmrv commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Answers your comment on #797, "Interactions with ssh cache unclear". Targets temppath.

The four inline review threads on #797 moved to #831, which has since mergedtemppath was merged back in here (f938975, no conflicts), so this PR is the SshCache × Path work only.

What the interaction actually was

Probed with a real python -m fleche remote --serve subprocess, client and server in different working directories so the same relative name denotes different files on each side. Three behaviours, all silent:

before
server can see something at that name stores its bytes, returns its digest — digest(x) == save_value(x) broken, so the record is filed under a key no client recomputes, and a load returns the wrong content
server can see nothing there Indigestible surfaces from inside an RPC
loading a path stored remotely server materializes into a temp dir on its disk and sends back only the name — dangling here, and unlinked there as soon as its own reference dies

The last one is not fixable by holding a reference: PurePath.__reduce__ rebuilds from parts alone, so TempPath's _temp_root is dropped in transit and _live_roots is per-process. The save_value comment already said "paths over SSH remain unsupported" — nothing enforced it.

What this does

Refuses, in both directions, via a new RemotePathUnsupported. It subclasses SaveError, so the two-phase-save degradations already in the codebase carry it and no caller special-cases anything:

  • path argument → digest-only reference, digest computed locally. The seal stays intact, lookups hit and miss correctly; only the bytes aren't retrievable from the remote record.
  • path resultRejected. The call runs, returns the file, is logged as not cached.
  • load → raises, lazily. A record whose result is a path still loads and queries; only touching the path value raises.

Detection is storage.paths.find_path, walking a value through a new storage.destructuring.child_slots — the read-only half of _intern_rec's dispatch — so nesting is covered exactly as far as a destructuring save reaches, and no further (a path inside an opaque object is still stored by location, same as locally).

Escape hatches, both documented: return bytes, or put a local layer in front of the remote one so saves never reach the wire.

Follow-up (second commit)

Per your review, the problem itself is tracked as #829 rather than left as a comment. The shape is already implied by PathValueMixin: it reduces a path to bytes plus a FileBlob/DirectoryBlob, all of which ship fine, and those blobs' __digest__ is defined to match the Path digest arm — so running the reduction client-side keeps the seal intact by construction, and needs no server change for saves (a FileBlob isn't a Path, so it falls straight through the server's path layer). The one genuinely new verb is an unmended load_value that returns the blob instead of materializing it on the server. Recorded in RemotePathUnsupported, dev/path_storage, and the DEVELOPING design-theme cheat sheet.

Docs

Tests

tests/unit/test_remote.py — bare and nested (list/tuple/nested-dict/dataclass) rejection with nothing reaching the server; the bytes hatch still working; prepare() keeping prepared.key == call.to_lookup_key() for a path argument (the regression the guard buys); live-Call rejection; load-side rejection incl. nested; a LazyCall whose arguments still read fine and only .result raises.

tests/unit/storage/test_paths.pyfind_path descent, opaque-leaf boundary, cycle termination.

tests/integration/test_remote.py — the divergent-cwd reproduction above, and a path-returning @fleche function running uncached against a remote instead of breaking.

Re-verified after merging temppath: 1788 passed / 11 skipped, docs build succeeds. ty check src/ reports 3 diagnostics — all already on temppath, none from this PR; see the #831 thread for the diagnosis and a verified fix, still pending your call on where it should land.

Path values are stored by content, but the RPC ships values by cloudpickle
and a pickled Path is only its string.  A path handed to the remote was
therefore a name the *server* resolved against its own filesystem:

* if a file sat at that name, the server stored those bytes under their
  digest — breaking `digest(x) == save_value(x)`, so the record landed under
  a key no client ever recomputes, and a load returned the wrong content;
* if nothing sat there, `Indigestible` surfaced from inside an RPC;
* on load the server materialized into a temp dir on its own disk and sent
  back only the name — dangling here, and unlinked there as soon as the
  server's own reference died (`PurePath.__reduce__` rebuilds from `parts`,
  so `TempPath`'s temp-root guard does not survive the hop).

Refuse instead, in both directions, via the new `RemotePathUnsupported`.
It subclasses `SaveError`, so the existing two-phase-save degradations
carry it without any caller special-casing: a path argument falls back to a
digest-only reference whose digest is computed locally (the seal stays
intact and lookups still hit), and a path result becomes `Rejected` — the
call runs and returns normally, just uncached.

Detection uses `storage.paths.find_path`, which walks a value through the
new `storage.destructuring.child_slots` — the read-only half of
`_intern_rec`'s dispatch — so nesting is covered exactly as far as a
destructuring save would reach, and no further.

Documents the contract in the file-semantics page (plus a quick-reference
row and a recipe) and the mechanism in the path-storage dev page, including
what full path support over SSH would additionally require.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RascUYS7JgCMHHpXsYbPTf

pmrv commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

CI is red on triage, but not because of this diff — GitHub Actions is failing to serve the action itself:

Prepare all required actions
Getting action download info
Failed to resolve action download info. Error: Service Unavailable
##[error]Service Unavailable

The job never reaches checkout, so no code in this PR is involved. Same failure on the original run and on a re-run ~7 min later, so the outage is ongoing. Collateral damage on the same commit: the ty workflow's changes gate sat queued for 15 min and was cancelled, which skipped ty itself.

Nothing to fix here. I'll re-run triage and ty once the service recovers, and report if either fails for a real reason.

For the record, locally on afb6851: full suite 1777 passed / 11 skipped, and ty check src/ at 4 diagnostics — the same 4 already present on temppath before this branch.


Generated by Claude Code

@pmrv pmrv left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impl ok for now, but let's track the problem in an issue. I think this should be solvable.

pmrv commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Tracked as #829 — agreed it's solvable, and the shape is fairly clear: PathValueMixin.save already reduces a path to bytes + FileBlob/DirectoryBlob, which ship fine, and those blobs' __digest__ is defined to match the Path digest arm. So running the reduction client-side keeps the seal intact by construction, and needs no server change for the save direction (a FileBlob isn't a Path, so it falls straight through the server's path layer). The one genuinely new verb is an unmended load_value that returns the blob instead of materializing it on the server, so the client materializes into its own temp dir and owns the TempPath lifetime. Round-trip count and a contains-batch to skip bytes the server already has are the perf questions; issue has the details. Referenced from remote.py, dev/path_storage, and the DEVELOPING theme list.

Also pushed e4a6f8e addressing the four inline threads on #797:

"feels almost more like it should go into a more generic section" — you were right that it isn't file-specific. Verified it generalizes (a mutated list argument behaves identically: cold call mutates, hit leaves the argument untouched, mutated-and-returned is stored post-mutation), so it's now usage/purity — keyed-as-passed, mutation not replayed, side effects cold-call-only — with file_semantics keeping only the path instance and linking out.

"cross ref destructuring here, since that's the key" — done, and reframed the section around it: only destructured children reach the path machinery, so _DESTRUCTURERS is the list of places a nested path gets content treatment. That one fact now explains the container list, the exact-types caveat, and the opaque-container section below it. Links to :ref:extending-destructurer``.

"move the completedprocess digester into fleche itself" — now a match arm in digest.py (args, returncode, stdout, stderr). Purely additive: those values raised Indigestible before, so no stored digest changes and no hash_version bump. Handles text-mode str and uncaptured None streams. The notebook's add_hook cell is gone and the pipeline section now just works.

"save executed version"Files.ipynb committed executed. While there: PathsInContainers.ipynb had been added with no docs/notebooks/ symlink, no toctree entry, and no slot in test_notebooks.py — wired up all three and committed it executed too.

Verification: 1788 passed / 11 skipped. ty check src/ at the same 4 pre-existing diagnostics. Docs build succeeds — and a correction to my earlier note on this PR: the builds I called clean before had actually been dying at notebooks/CacheStack.ipynb for want of pandoc and never reached the pages I'd changed. With pandoc installed the full build succeeds and the only non-autoapi warning is the pre-existing fleche.D duplicate; I fixed a short title underline in usage/helpers that turned up once the build got that far.


Generated by Claude Code

@pmrv

pmrv commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

separate PRs for both commits

The guard added in the previous commit refuses Path values rather than
solving them; per review, the problem itself is now tracked.  The shape is
already implied by PathValueMixin: it reduces a path to bytes plus a
FileBlob/DirectoryBlob, all of which ship fine over the wire, and those
blobs' `__digest__` is *defined* to match the Path digest arm — so running
the reduction client-side keeps the seal intact by construction.  The one
genuinely new piece is an unmended `load_value` that returns the blob
instead of materializing it on the server.

Referenced from `RemotePathUnsupported`, the path-storage dev page, and the
DEVELOPING design-theme cheat sheet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RascUYS7JgCMHHpXsYbPTf

pmrv commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Split — the four inline threads are now #831, this PR is the SshCache × Path work only.

The split isn't quite along the commit boundary, because the second commit carried three hunks that were about this PR: the #829 note in RemotePathUnsupported, the #829 rewrite of the dev/path_storage paragraph, and the "Distributed / remote caching" design-theme entry. Those stayed here as c047dd3 — they're the answer to your "let's track the problem in an issue", so they belong with the guard, not with the docs work.

Result: afb6851 (guard) + c047dd3 (record #829) here; everything else on #831. No shared hunks between the two branches — both are cut from temppath and either can land first.

Re-verified after the split: 1777 passed / 11 skipped here, 1763 on #831 (the two add 25 and 11 tests to temppath's baseline respectively, which reconciles with the 1788 the combined branch reported). ty unchanged at 4 pre-existing diagnostics on both; docs build succeeds on both.


Generated by Claude Code

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

🤖 CI failure summary for afb6851c on claude/temppath-comment-r1zhhx

Summary
The workflow failed due to a runner acquisition issue. The changes job was cancelled because no hosted runner of the required type could be acquired after multiple attempts, causing the dependent ty job to be skipped.

Root cause
GitHub Actions runner unavailability:

The job was not acquired by Runner of type hosted even after multiple attempts

Suggested fix

  1. Retry the run: gh run rerun 31120548574 --repo pmrv/fleche
  2. If retries continue to fail, check GitHub's status page for runner capacity issues
  3. If this is a custom runner requirement in the workflow, verify the runner is available and healthy

Generated by Claude Haiku 4.5.

…omment-r1zhhx

# Conflicts:
#	src/fleche/storage/paths.py
@pmrv
pmrv merged commit 0ddf18f into temppath Aug 6, 2026
@pmrv
pmrv deleted the claude/temppath-comment-r1zhhx branch August 6, 2026 20:59
pmrv pushed a commit that referenced this pull request Aug 6, 2026
Conflict in `SshCache`, in `remote.py` — and not only textual.  Both sides
changed `save`, and `prepared-call` adds `prepare`, an RPC that carries
argument *values* rather than digests.  That is a fourth way a `Path` can
reach the server, which the guard added in #828 did not cover: shipping the
live call hands the far side a path *string* to resolve against its own
filesystem, breaking `digest(x) == save_value(x)` exactly as before.

Resolved by keeping both sides and closing that route:

- `save` handles `PreparedCall` (ship result, then file the record) as well
  as the live-`Call` and `DigestedCall` forms, routing the result through
  `save_value` rather than `_rpc` so the path guard applies to it.
- `prepare` falls back to the local `BaseCache` two-phase prepare when any
  argument carries a path.  Each argument then goes through `save_value`,
  where a path degrades to a digest-only reference computed *locally* — the
  seal stays intact and lookups still hit, which is the same degradation
  #828 established for the other routes.

Also widens `find_path` itself, which was too narrow to be safe.  It mirrored
*destructuring*, but the invariant at stake is `digest(x) == save_value(x)`,
and `digest` recurses further than destructuring does: into namedtuples,
sets, and frozensets, reading the file in each.  A path hidden in one of
those therefore still decides the key while passing the guard, so
`Bundle(path, 0.5)` crossed the wire and the server digested that name
against its own disk.  The walk now follows `digest` instead, stopping where
`digest` stops (an arbitrary object is `Indigestible`, so no path inside it
can decide a key) and declining to consume one-shot iterables.

1807 passed, 11 skipped; `ty check src/` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RascUYS7JgCMHHpXsYbPTf
@pmrv
pmrv restored the claude/temppath-comment-r1zhhx branch August 7, 2026 21:40
pmrv added a commit that referenced this pull request Aug 8, 2026
)

## Summary

Audit sweep over `agents/DEVELOPING.md`. Records the recent code changes
since the last docs sync (`cad32cd`) so the file stays a reliable
starting point for a fresh session.

### Landed since last sync

- **PR #793** (`feat(caches)!`, released as 2.0.0). New two-phase save
protocol — `BaseCache.prepare(call) → PreparedCall`; `Cache.prepare`
stashes arguments into `self.values` immediately so the recorded lookup
key describes the arguments as they were *before* the body ran (an
argument-mutating body could previously leak its post-mutation state
into the record). `PreparedCall.commit(result, metadata)` calls
`cache.save(self)` on the sealed record; `.abandon()` releases without
filing; `.resolve(values)` stores the pending result into a value
storage (used by `Cache.save` and by `SshCache.save`'s two-trip commit).
`CacheWrapper.prepare` rebinds `PreparedCall.cache=self` so wrapper
policy still governs the commit; `CacheStack.prepare` delegates to
`stack[0]`; `ReadOnlyMixin.prepare` seals the key with a digest-only
admission so the body still runs uncached. Wrapper (`wrapper.py`)
integrates the protocol with a fallback to uncached execution on
`prepare` failure and `prepared.abandon()` around every exception path.
On the wire, `SshCache.prepare` is one RPC returning the sealed
`DigestedCall`; `SshCache.save(prepared)` is two RPCs (`save_value` +
`save`) — a `PreparedCall` never crosses the wire.

- **PR #818** (`test(caches)`). Pins the corrupt-cache silent-skip
contract in `Cache._query` — an exploding `DigestedCall.fetch` in the
middle of a query stream now provably logs at ERROR on `fleche.cache`
and continues rather than aborting iteration. Test lives in
`tests/unit/caches/test_cache.py::test_cache_query_logs_and_skips_calls_that_fail_to_fetch`.

- **PR #839** + follow-ups. All eight notebooks are symlinked into
`docs/notebooks/` and executed by `test_notebooks.py`; the test
parametrisation flipped from a hand-maintained five-file list to
`sorted(NOTEBOOKS_DIR.glob("*.ipynb"))`. `ConcurrentExecution.ipynb` is
pinned to `multiprocessing.get_context("fork")` to survive Python 3.14's
`forkserver` default.

### Open-scope updates

- 2026-08-07 refactor batch (#832 split `remote.py`, #833 collapse
`to_config`/`register_storage` boilerplate, #834 turn `_digest_bytes`
`match/case` into a dispatch table, #835 hoist
`_redact_config`/`_redact_url_password` to `config.py`).
- Bugs #826 (`gc()` can collect in-flight `PreparedCall` argument
values) and #840 (`BoundWrapper` only survives `ProcessPoolExecutor`
under `fork`).
- Feature request #829 (`SshCache` `Path`-value support — PR #828 has
the refusal on the `temppath` feature branch, not on `main`).

## Test plan

- [x] `git diff --stat` shows `agents/DEVELOPING.md` only.
- [x] Every added claim spot-checked against source: `PreparedCall` at
`src/fleche/call.py:335`, `Cache.prepare` at `src/fleche/caches.py:322`,
`SshCache.prepare` at `src/fleche/remote.py:859`, `_save_value` at
`src/fleche/remote.py:145`, `test_prepared_call.py` present,
corrupt-cache test at `tests/unit/caches/test_cache.py`,
`test_notebooks.py` glob, `docs/notebooks/` symlink set.
- [x] Open-issue numbers verified via `list_issues` (state=OPEN) against
`pmrv/fleche` on 2026-08-08.

---
_Generated by [Claude
Code](https://claude.ai/code/session_014DeWPsJ97GnPLhauttvM6N)_

Co-authored-by: claude[bot] <claude[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants