fix: keep a binding's stamped id when this run cannot resolve it - #749
Closed
raymondk wants to merge 1 commit into
Closed
fix: keep a binding's stamped id when this run cannot resolve it#749raymondk wants to merge 1 commit into
raymondk wants to merge 1 commit into
Conversation
`set_env_vars_for_canister` wrote the canister's manifest variables plus the bindings it could resolve, and `update_settings` replaces the whole list. So a binding whose target had no id in the environment's store was not merely skipped — the variable a full deploy had stamped was deleted, and the canister read an empty value from then on, with nothing said anywhere. An unresolved binding means "no id in *this* store", not "not wired": the referenced canister lies outside the run's scope, or was deployed from another project root, which is what a workspace member deployed on its own sees of its dependencies. The `resolve_targets` guard catches one shape of this (a member-scoped deploy that names no canisters) and does not catch the rest, because naming canisters explicitly skips it. Carry the current value over for a binding this run cannot compute, and warn with the canister that could not be resolved. Resolved bindings still overwrite, and a variable the manifest declares still outranks a stamped one, so nothing else about the namespace changes. The canister's status is read only when there is an unresolved binding, so the common path still writes without reading first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Closing this one. The state it guards — the consumer's id present in a store where the dependency's id is absent — is much narrower than the description implied. In a well-formed workspace, root resolution climbs from the member to the workspace root, so a per-service deploy reads the root's store and the binding resolves. Reaching the unresolvable state needs the dependency to have been deleted, its store key to have changed, the environment to exclude it, or root resolution not to climb. That makes this defensive rather than the fix for the reported issue, and the reported issue is better explained by #748. Reopening later is cheap if we decide the invariant is still worth enforcing — the branch |
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
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.
Problem
set_env_vars_for_canisterbuilds a canister's variable list as manifest variables + the bindings it could resolve, andupdate_settingsreplaces the whole list. A binding whose target had no id in the environment's store was dropped by afilter_mapwith no warning — so the variable a full deploy had stamped was deleted, and the canister read an empty value from then on.An unresolved binding means "no id in this store", not "not wired": the referenced canister lies outside the run's scope, or was deployed from another project root — which is what a workspace member deployed on its own sees of its dependencies.
resolve_targetscatches one shape of this (a member-scoped deploy that names no canisters) and misses the rest, because naming canisters explicitly skips the guard.Reported from the field as a consumer canister silently losing its provider id across per-service deploys, worked around by re-asserting the values with
settings update --add-environment-variableafter every one.Fix
Resolved bindings still overwrite, and a manifest-declared value still outranks a stamped one, so nothing else about the
PUBLIC_CANISTER_ID:*namespace changes — a variable for a binding that no longer exists is still pruned.Test
scoped_deploy_keeps_a_dependency_id_it_cannot_resolvedeploys a workspace, drops the dependency's entry from the id store (the state a service deployed from its own root is in), then redeploys just the consumer by name.Without the fix,
settings showafterwards holds onlyPUBLIC_CANISTER_ID:app— the dependency variable is gone, which is the reported bug reproduced. With it, the id survives and the run explains what it could not resolve.Checks
cargo test -p icp --lib(310),--test dependency_tests(7),--test canister_settings_tests(13),--test deploy_tests(24 of 25),cargo fmt,cargo clippy --all-targets. The one failure isdeploy_cloud_engine::docker::test, which needs a Docker daemon that is not running locally; it fails the same way onmain.Not covered here
icp canister settings syncstill never stampsPUBLIC_CANISTER_ID:*at all — bindings are computed only on the deploy path — so it can neither add a missing dependency id nor correct a stale one. Whether sync should also own that namespace is a design question worth settling separately.🤖 Generated with Claude Code