feat(llard): reuse workspace artifacts before Kodo - #202
Conversation
llard consulted Kodo on every build-cache lookup, downloading and unpacking the artifact even when it was already present in the workspace. Add a llard-local read-through cache so llard serves the workspace copy first and only falls back to Kodo on a miss. The workspace is evictable, so Kodo stays the source of truth: a remote hit is persisted into the local cache for later reads. The implementation is a deliberate copy of the client's read-through cache so llard and the client do not share an abstraction. Only cmd/llard changes; the llar CLI and shared packages are untouched.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: read-through cache for llard
The change is small and well-scoped: llard now wraps its Kodo cache in a local-first readThroughCache, matching the existing client-side pattern in cmd/llar/internal/make.go. The read-through logic is correct for the current consumers, the doc comments accurately describe behavior (verified against kodo.go and internal/build/cache.go), and the tests meaningfully assert behavior via call counts rather than just return values. Nice.
A few points worth considering below — none are blocking.
Design note (not inline): the local-first path trusts a local hit and returns immediately, whereas the remote path verifies a SHA-256 checksum on restore (kodo.go). So on a local hit the daemon serves an entry that was never re-validated against Kodo. The top comment frames the local cache as best-effort with "Kodo remains the source of truth", but on a hit the local copy is effectively authoritative. This only matters if something other than a verified remote restore can write into the shared workspace — worth confirming it's intended.
Tests (not inline): the error branches in Get (remote error at main.go:140, and local.Put failure after a remote hit at main.go:146-148) are not exercised — countingCache.err is declared but never set. A case asserting the remote error propagates (and local is not written), plus one asserting a post-hit local Put failure yields Entry{}, false, err rather than a spurious hit, would close the gap.
| } | ||
| // The remote store already restored the artifact into the shared | ||
| // workspace, so the local cache only needs to persist its entry. | ||
| entry, err = c.local.Put(ctx, key, nil, entry) |
There was a problem hiding this comment.
local.Put here persists only Metadata (and BuildTime); it drops entry.Deps. On a remote hit the Kodo Get returns Entry{Metadata, Deps}, so the first call returns Deps (the pre-Put entry at line 149) while a later local hit — localCache.Get returns Entry{Metadata: ...} only — returns no Deps. That first-vs-later asymmetry is harmless today because the only consumer (http.build) reads entry.Metadata and reconstructs Deps from the module graph, but it's a latent trap for any future consumer that starts relying on cached Deps. Worth a short comment stating the local cache intentionally does not round-trip Deps, or closing the gap in localCache.Put.
| if err != nil || !ok { | ||
| return entry, ok, err | ||
| } | ||
| // The remote store already restored the artifact into the shared |
There was a problem hiding this comment.
This adds a write to the read path: every remote hit now does a full .cache.json read-modify-write via localCache.Put. That file is keyed by module path only (workspaceDir/<escapedModulePath>/.cache.json), shared across all versions/matrices of a module, and the write is non-atomic (load → mutate → os.WriteFile). In llard the top-level singleflight only dedupes by the root request key, not by shared dependencies, so concurrent requests for different roots that share a dependency (or different matrices of one module) can race on this file — last-writer-wins, plus torn reads that localCache.load swallows as a miss and silently discards. Put was already on the build path, but routing it onto the read path widens the window. Consider an atomic write (temp file + rename) or a per-module-path lock.
15db543 to
ed5f7d8
Compare
There was a problem hiding this comment.
Review: read-through cache for llard
The read-through cache is small, well-structured, and the local-first / remote-fallback / persist-on-remote-hit ordering is correct. The doc comment explaining the deliberate duplication of the client's cache is a good "why" comment, and the three tests cover the meaningful behaviors. One behavioral change stands out and is worth confirming it's intended (P1), plus two lower-severity notes.
Main question — does llard still publish builds to Kodo?
Before this PR, buildCache was the Kodo cache directly, so a build cache-miss (Builder.Build → Cache.Put(ctx, key, os.DirFS(installDir), entry) at internal/build/build.go:360) uploaded the artifact to Kodo. After this PR, readThroughCache.Put routes only to c.local, and it is the only Cache.Put callsite reachable from the daemon. So llard-performed builds are no longer uploaded to Kodo. If that's intended (clients publish; llard only serves + rebuilds locally), a note in the doc comment would help — the current "Kodo remains the source of truth" wording reads as if the write path still populates it. If not intended, the remote needs a write on Put.
Minor: request version (parsed after @ in internal/build/http/http.go) flows unescaped into on-disk installDir names (internal/build/cache.go, kodo.go) — pre-existing, but this cache extends reliance on that key derivation; worth validating versions at the request boundary in a follow-up.
| } | ||
|
|
||
| func (c readThroughCache) Put(ctx context.Context, key cache.Key, output fs.FS, entry cache.Entry) (cache.Entry, error) { | ||
| return c.local.Put(ctx, key, output, entry) |
There was a problem hiding this comment.
Put never writes to the remote — llard no longer populates Kodo on build.
This is the only Cache.Put callsite the daemon reaches (via Builder.Build, internal/build/build.go:360, called with the built os.DirFS(installDir) on a cache miss). Before this PR buildCache was the Kodo cache directly, so completed builds were uploaded to Kodo. Now they are written only to the evictable local workspace.
Consequence: after the local entry is evicted or the daemon starts on fresh infrastructure, the artifact is gone and must be rebuilt from source rather than restored from Kodo, and the public cache is never populated by daemon builds. Please confirm this is intended. If clients are the sole publishers, add a line to the doc comment saying so; if not, Put should also write through to c.remote.
| } | ||
| entry, ok, err = c.remote.Get(ctx, key) | ||
| if err != nil || !ok { | ||
| return entry, ok, err |
There was a problem hiding this comment.
On a local miss, a non-nil error from c.remote.Get is returned directly, so a transient Kodo/artifact-store transport error surfaces as a hard failure. The client reference (cmd/llar/internal/make.go) has the same shape but its remote is the public Kodo path (getPublic), which deliberately converts transport failures into cache misses so callers fall back to a source build (internal/build/cache/kodo.go). llard wires the credentialed path, where transport errors are returned as-is — so the copied code has different effective semantics here. Consider whether a remote error should degrade to a miss (as the client does) or document that it is intentionally fatal for the daemon.
| } | ||
| // The remote store already restored the artifact into the shared | ||
| // workspace, so the local cache only needs to persist its entry. | ||
| entry, err = c.local.Put(ctx, key, nil, entry) |
There was a problem hiding this comment.
localCache.Put persists only Metadata, not Deps (buildEntry in internal/build/cache.go). So the first read here returns the full remote entry (Put returns the passed-in entry unchanged), but a later local hit returns Deps: nil. Harmless in the current build path (the cache-hit fast path at build.go:277 reads only entry.Metadata), but the doc comment's "persisted back into the local cache for later reads" slightly overstates fidelity. Worth a note, or ignore if Deps is never consumed on a cache hit.
The llard read-through cache copied the client's Put, which only writes the local entry. On a cold build that skipped the remote Put entirely, so no artifact was uploaded or recorded and the handler failed with 'artifact not found'. Publish remotely first: on success cache the authoritative entry locally; on failure (another llard already published it) do not cache this build's copy and let the next Get restore the canonical artifact.
Deleting a published artifact only removes the Kodo object and its record; the worker keeps its local workspace entry. A pure local-first read-through therefore served the deleted artifact, skipped the rebuild, and the handler failed the response with 'artifact not found'. Check the remote artifact record before trusting the local entry: a missing record reports a cache miss so the module is rebuilt, while a present record still serves the local copy without downloading the artifact.
A deleted remote record makes the local copy stale, but the rebuild only ran os.MkdirAll over the existing directory, so removed files could linger and be packed into the new artifact. Remove the workspace install tree on the record-missing miss so the rebuild starts clean.
2 participants
llard consulted Kodo on every build-cache lookup, downloading and unpacking the artifact even when it was already present in the workspace. Add a llard-local read-through cache so llard serves the workspace copy first and only falls back to Kodo on a miss.
The workspace is evictable, so Kodo stays the source of truth: a remote hit is persisted into the local cache for later reads. The implementation is a deliberate copy of the client's read-through cache so llard and the client do not share an abstraction.
Only cmd/llard changes; the llar CLI and shared packages are untouched.