You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up to dodder#352, which fixed 17 confirmed content-addressed-read call sites pinned to the single-store env_repo.Env.GetDefaultBlobStore() instead of the multi-store-fallback GetReadBlobStore(). That pass deliberately left local_working_copy.Repo.GetBlobStore() (a thin wrapper around GetDefaultBlobStore()) untouched, since it has at least one legitimate write caller and several other callers whose intent wasn't traced. This issue now also covers auditing and resolving that wrapper's callers, not just the rename.
Structural finding (blocks a simple fix)
GetBlobStore() is part of the repo.Repo interface contract
(go/internal/papa/repo/main.go:23):
The return type is the concrete structblob_stores.BlobStoreInitialized (which carries .Path/.Config fields), not the mad_domain_interfaces.BlobStore interface GetReadBlobStore() returns. This means the #352-style fix — swap the accessor call — does not work here. local_working_copy.Repo.GetBlobStore() cannot simply return GetReadBlobStore()'s value; the interface contract requires the struct shape. A real fix needs either:
Wrap the multi-store Multi inside a BlobStoreInitialized-shaped value (its .Path/.Config presumably describing the default store while its embedded blob-store behavior routes through Multi) — requires understanding madder's BlobStoreInitialized struct definition, which isn't in this repo (amarbel-llc/madder module) or cached locally; needs a source read from that repo.
Change the repo.Repo interface's GetBlobStore() signature to return mad_domain_interfaces.BlobStore instead — larger blast radius, touches both known implementations (local_working_copy.Repo, remote_http.client — see below) and every caller that currently relies on .Path/.Config.
remote_http.client.GetBlobStore() (client_blob_store.go:23-30) → constructs a fresh BlobStoreInitialized representing the blob store as seen through this specific HTTP connection to one remote. No local-disk multi-store concept applies here — it's inherently singular by what it represents, not a bug candidate.
Since GetBlobStore() is called through the repo.Repo interface at most call sites, the same line of code can resolve to either implementation depending on the concrete type passed at runtime. Only call sites where the concrete type is (or can be) local_working_copy.Repo are in scope.
Callers traced so far — READ candidates (not yet fixed, blocked on the structural issue above)
go/internal/romeo/local_working_copy/local_op_pull.go:45 — remote.GetBlobStore() passed to store.MakeEdgeExplorer(...), used for reference/presence checks while expanding the pull's edge closure.
go/internal/sierra/remote_proto/transfer.go:81 — src.GetBlobStore(), same MakeEdgeExplorer pattern, drtp transport's sender side (sendClosure, used by both fetch-serve and push-send — src is always the local/sending repo per the package's own doc comments).
go/internal/uniform/commands_dodder/init_workspace.go:479 — remoteBlobStore := remote.GetBlobStore(), later calls .MakeBlobReader(blobDigest) directly (line 510) to dereference inventory-list blob contents during workspace init.
go/internal/sierra/remote_http/client.go:326 — remote.GetBlobStore() passed into client.WriteBlobToRemote(localBlobStore, ...), whose first parameter is read (.MakeBlobReader) to stream content the peer requested — despite the local variable being named remote, this reads the local side's own blob content.
go/internal/uniform/commands_dodder/clone.go:222,394 — remote.GetBlobStore() / source.GetBlobStore() used as src in CopyBlobIfNecessary, same class as the above (reading the source repo's own blobs during a copy).
Confirmed correct as-is (config-only, not a content read)
local_op_pull.go:91-92 — remote.GetBlobStore() used as src, local.GetEnvRepo().GetDefaultBlobStore() used as dst, in a CopyBlobIfNecessary call — dst is correctly write-pinned; src falls into the same READ-candidate bucket as the clone.go sites above.
golf/blob_transfers/main.go:135-136 — dst.GetBlobStore() / blobImporter.Src.GetBlobStore() are calls on blob_stores.BlobStoreInitialized itself (an unrelated method on a different type from the same package), not on repo.Repo — not in scope for this issue.
Ask
Read blob_stores.BlobStoreInitialized's definition in amarbel-llc/madder to determine whether option 1 above (wrap Multi in a BlobStoreInitialized-shaped value) is viable.
Decide between option 1 and option 2 (interface signature change) — or some third approach.
Once unblocked, migrate the confirmed READ candidates above.
Fold in the original rename/restrict-GetDefaultBlobStore() ask from this issue's first draft.
Context
Follow-up to dodder#352, which fixed 17 confirmed content-addressed-read call sites pinned to the single-store
env_repo.Env.GetDefaultBlobStore()instead of the multi-store-fallbackGetReadBlobStore(). That pass deliberately leftlocal_working_copy.Repo.GetBlobStore()(a thin wrapper aroundGetDefaultBlobStore()) untouched, since it has at least one legitimate write caller and several other callers whose intent wasn't traced. This issue now also covers auditing and resolving that wrapper's callers, not just the rename.Structural finding (blocks a simple fix)
GetBlobStore()is part of therepo.Repointerface contract(
go/internal/papa/repo/main.go:23):The return type is the concrete struct
blob_stores.BlobStoreInitialized(which carries.Path/.Configfields), not themad_domain_interfaces.BlobStoreinterfaceGetReadBlobStore()returns. This means the #352-style fix — swap the accessor call — does not work here.local_working_copy.Repo.GetBlobStore()cannot simply returnGetReadBlobStore()'s value; the interface contract requires the struct shape. A real fix needs either:Multiinside aBlobStoreInitialized-shaped value (its.Path/.Configpresumably describing the default store while its embedded blob-store behavior routes throughMulti) — requires understanding madder'sBlobStoreInitializedstruct definition, which isn't in this repo (amarbel-llc/maddermodule) or cached locally; needs a source read from that repo.repo.Repointerface'sGetBlobStore()signature to returnmad_domain_interfaces.BlobStoreinstead — larger blast radius, touches both known implementations (local_working_copy.Repo,remote_http.client— see below) and every caller that currently relies on.Path/.Config.Two implementations found, only one is ambiguous
local_working_copy.Repo.GetBlobStore()(accessors.go:92-94) →env.GetDefaultBlobStore(). This is the one with the single-vs-multi-store ambiguity Audit GetDefaultBlobStore() read call sites; inventory_list_store.Store.blobBlobStore bypasses FDR-0015 multi-store fallback #352 is about.remote_http.client.GetBlobStore()(client_blob_store.go:23-30) → constructs a freshBlobStoreInitializedrepresenting the blob store as seen through this specific HTTP connection to one remote. No local-disk multi-store concept applies here — it's inherently singular by what it represents, not a bug candidate.Since
GetBlobStore()is called through therepo.Repointerface at most call sites, the same line of code can resolve to either implementation depending on the concrete type passed at runtime. Only call sites where the concrete type is (or can be)local_working_copy.Repoare in scope.Callers traced so far — READ candidates (not yet fixed, blocked on the structural issue above)
go/internal/uniform/commands_dodder/diff.go:104—localWorkingCopy.GetBlobStore()fed intoobject_metadata_fmt_hyphence.Factory.BlobStoreforMakeFormatterFamily()'sInlineBlobread path — the exact pattern already fixed intyped_blob_store/text_formatter.goby Audit GetDefaultBlobStore() read call sites; inventory_list_store.Store.blobBlobStore bypasses FDR-0015 multi-store fallback #352.go/internal/romeo/local_working_copy/local_op_pull.go:45—remote.GetBlobStore()passed tostore.MakeEdgeExplorer(...), used for reference/presence checks while expanding the pull's edge closure.go/internal/sierra/remote_proto/transfer.go:81—src.GetBlobStore(), sameMakeEdgeExplorerpattern, drtp transport's sender side (sendClosure, used by both fetch-serve and push-send —srcis always the local/sending repo per the package's own doc comments).go/internal/uniform/commands_dodder/init_workspace.go:479—remoteBlobStore := remote.GetBlobStore(), later calls.MakeBlobReader(blobDigest)directly (line 510) to dereference inventory-list blob contents during workspace init.go/internal/sierra/remote_http/client.go:326—remote.GetBlobStore()passed intoclient.WriteBlobToRemote(localBlobStore, ...), whose first parameter is read (.MakeBlobReader) to stream content the peer requested — despite the local variable being namedremote, this reads the local side's own blob content.go/internal/uniform/commands_dodder/clone.go:222,394—remote.GetBlobStore()/source.GetBlobStore()used assrcinCopyBlobIfNecessary, same class as the above (reading the source repo's own blobs during a copy).Confirmed correct as-is (config-only, not a content read)
command_components_dodder/remote.go:585,623,remote_http/client.go:242,remote_http/server.go:573(pre-Audit GetDefaultBlobStore() read call sites; inventory_list_store.Store.blobBlobStore bypasses FDR-0015 multi-store fallback #352),remote_http/round_tripper_unix_socket.go:24,remote_http/sig_roundtrip_test.go:96— all only call.GetDefaultHashType()on the result, never read blob content by digest.local_op_pull.go:91-92—remote.GetBlobStore()used assrc,local.GetEnvRepo().GetDefaultBlobStore()used asdst, in aCopyBlobIfNecessarycall —dstis correctly write-pinned;srcfalls into the same READ-candidate bucket as the clone.go sites above.remote_proto/transfer.go:441—dst.GetBlobStore(), explicitly namedwriteBlobStore, correctly write-pinned (the confirmed non-bug that made Audit GetDefaultBlobStore() read call sites; inventory_list_store.Store.blobBlobStore bypasses FDR-0015 multi-store fallback #352 cautious about touching the wrapper in the first place).golf/blob_transfers/main.go:135-136—dst.GetBlobStore()/blobImporter.Src.GetBlobStore()are calls onblob_stores.BlobStoreInitializeditself (an unrelated method on a different type from the same package), not onrepo.Repo— not in scope for this issue.Ask
blob_stores.BlobStoreInitialized's definition inamarbel-llc/madderto determine whether option 1 above (wrapMultiin aBlobStoreInitialized-shaped value) is viable.GetDefaultBlobStore()ask from this issue's first draft.:clown: Updated by Clown 0.3.19+f1e3833 (build).