[bug-7acxk8rf0g6b] fix(site-publish): close split migration boundaries #4

Merged
architect merged 5 commits from architect/bug-7acxk8rf0g6b/postmerge-contract-fixes into main 2026-08-29 23:25:55 +00:00
3 changed files with 79 additions and 27 deletions
Showing only changes of commit 892b6e6441 - Show all commits
+4 -2
View File
@@ -133,8 +133,10 @@ any route's mutable objects change.
Mutable default and override partitions receive their final cache policy before Mutable default and override partitions receive their final cache policy before
the matching prefix-scoped stale deletion, so publication never exposes a the matching prefix-scoped stale deletion, so publication never exposes a
provisional cache policy or a pointer to a missing immutable target. provisional cache policy or a pointer to a missing immutable target.
Generated Ingress annotations retain each bucket's prior route and immutable paths. When a move Generated Ingress annotations retain each bucket's prior route and all immutable
places that retired prefix inside the new sync scope, only its declared key prefixes. The prefixes are bucket-relative and carried forward after a rule
is removed or renamed, so later deployments and route moves cannot forget them.
When a move places a retired prefix inside the new sync scope, only its declared
immutable subtrees are excluded; a current-file collision fails publication. immutable subtrees are excluded; a current-file collision fails publication.
The bucket-keyed history rejects a protected-to-public transition even when the The bucket-keyed history rejects a protected-to-public transition even when the
artifact is renamed; publishing that artifact publicly requires a new bucket. artifact is renamed; publishing that artifact publicly requires a new bucket.
+36 -21
View File
@@ -178,29 +178,40 @@ def publish_route_immutables(artifact, route, site_dir, credential_env_names=Non
publish_immutable_rule(artifact, route, rule, html_dir, aws_env) publish_immutable_rule(artifact, route, rule, html_dir, aws_env)
def immutable_key_prefixes(artifact, route):
"""Return immutable partitions as bucket-relative key prefixes."""
route_prefix = route["path"].strip("/")
return [
"/".join(part for part in (route_prefix, rule["path"]) if part)
for rule in artifact["cache_rules"] if _is_immutable(rule)
]
def retained_immutable_paths(artifact, route, previous_contract):
"""Carry all bucket history forward so later route moves cannot delete it."""
previous_paths = previous_contract["immutable_paths"] if previous_contract else []
return sorted(set(previous_paths) | set(immutable_key_prefixes(artifact, route)))
Outdated
Review

You move /foo to / while removing immutable releases; this rebuilds retirement filters from new rules, so sync --delete silently removes foo/releases/*. Clients and rollbacks lose those URLs. Persist prior immutable paths or stop before S3.

You move `/foo` to `/` while removing immutable `releases`; this rebuilds retirement filters from new rules, so `sync --delete` silently removes `foo/releases/*`. Clients and rollbacks lose those URLs. Persist prior immutable paths or stop before S3.
def retired_immutable_filters(artifact, route, html_dir, previous_contract): def retired_immutable_filters(artifact, route, html_dir, previous_contract):
"""Protect immutable keys only when an old route falls inside the new scope.""" """Protect historical immutable keys that fall inside the current sync scope."""
if not previous_contract: if not previous_contract:
return [] return []
current_prefix = route["path"].strip("/") current_prefix = route["path"].strip("/")
previous_prefix = previous_contract["path"].strip("/")
if current_prefix:
marker = f"{current_prefix}/"
if previous_prefix == current_prefix:
previous_prefix = ""
elif not previous_prefix.startswith(marker):
return []
else:
previous_prefix = previous_prefix[len(marker):]
filters = [] filters = []
current_immutable = { current_immutable = set(immutable_key_prefixes(artifact, route))
Outdated
Review

You move protected /portal to public /; this emits --exclude portal/releases/* without proving the old route shared the new access class. sync --delete retains the object, then the public catch-all serves it.

The narrowed prefix fixes collateral basename matches. The collision check only sees current local files. Neither establishes that a retired object is public.

Record and compare prior access class, or fail the move until explicit cleanup records that proof.

You move protected `/portal` to public `/`; this emits `--exclude portal/releases/*` without proving the old route shared the new access class. `sync --delete` retains the object, then the public catch-all serves it. The narrowed prefix fixes collateral basename matches. The collision check only sees current local files. Neither establishes that a retired object is public. Record and compare prior access class, or fail the move until explicit cleanup records that proof.
rule["path"] for rule in artifact["cache_rules"] if _is_immutable(rule)
}
for immutable_path in previous_contract["immutable_paths"]: for immutable_path in previous_contract["immutable_paths"]:
retired_path = "/".join(part for part in (previous_prefix, immutable_path) if part) if immutable_path in current_immutable:
continue
if current_prefix:
marker = f"{current_prefix}/"
if not immutable_path.startswith(marker):
continue
retired_path = immutable_path[len(marker):]
else:
Outdated
Review

You publish docs/releases/index.html; this wildcard silently excludes it from upload and deletion although only root /releases is immutable. A visitor keeps stale content, and the operator gets a successful deploy. Preserve only retired route prefixes.

You publish `docs/releases/index.html`; this wildcard silently excludes it from upload and deletion although only root `/releases` is immutable. A visitor keeps stale content, and the operator gets a successful deploy. Preserve only retired route prefixes.
Outdated
Review

*/releases/* also matches current files like docs/releases/x, while immutable publication covers only root releases/. Both upload passes skip valid default-cache content, leaving it absent or stale. Preserve retired keys during deletion without suppressing current uploads; add a nested-path regression.

`*/releases/*` also matches current files like `docs/releases/x`, while immutable publication covers only root `releases/`. Both upload passes skip valid default-cache content, leaving it absent or stale. Preserve retired keys during deletion without suppressing current uploads; add a nested-path regression.
Outdated
Review

Blocker: AWS CLI applies this wildcard to every descendant. With immutable releases, mutable archive/releases/app.js is excluded from upload and deletion, although only root releases owns immutable policy. Preserve actual retired prefixes; this basename wildcard silently strands current mutable content.

Blocker: AWS CLI applies this wildcard to every descendant. With immutable `releases`, mutable `archive/releases/app.js` is excluded from upload and deletion, although only root `releases` owns immutable policy. Preserve actual retired prefixes; this basename wildcard silently strands current mutable content.
Outdated
Review

You send GET /portal/releases/<digest> after this bucket moves from a protected /portal route to a public / route, and the public catch-all serves the formerly protected object because */releases/* excludes it from sync --delete. The current bucket/access check has a history-blind twin: it rejects simultaneous protected/public reuse but doesn't prove a retired prefix had the new route's access class. Content addressing prevents replacement, but it doesn't prevent this read. Preserve only retired prefixes proven to share the new access class, or fail the move until an explicit cleanup/migration records that proof.

You send `GET /portal/releases/<digest>` after this bucket moves from a protected `/portal` route to a public `/` route, and the public catch-all serves the formerly protected object because `*/releases/*` excludes it from `sync --delete`. The current bucket/access check has a history-blind twin: it rejects simultaneous protected/public reuse but doesn't prove a retired prefix had the new route's access class. Content addressing prevents replacement, but it doesn't prevent this read. Preserve only retired prefixes proven to share the new access class, or fail the move until an explicit cleanup/migration records that proof.
retired_path = immutable_path
collision = html_dir / retired_path collision = html_dir / retired_path
if (immutable_path not in current_immutable and collision.exists() if collision.exists() and any(path.is_file() for path in collision.rglob("*")):
and any(path.is_file() for path in collision.rglob("*"))):
raise RuntimeError( raise RuntimeError(
f"current artifact collides with retired immutable partition: {retired_path}" f"current artifact collides with retired immutable partition: {retired_path}"
) )
@@ -308,18 +319,20 @@ def ensure_bucket_aliases(site_name, aliases, admin_token):
raise raise
Outdated
Review

You remove releases while moving /foo to /; the first deployment reads the old annotation and excludes foo/releases/*, but this current-only list writes immutable-paths: []. On the next unchanged deployment, history contains no retired path, so sync --delete removes those objects and still reports success. Carry the retained immutable history forward, with enough route history to preserve its key prefix, and cover the removal deployment plus the following deployment.

You remove `releases` while moving `/foo` to `/`; the first deployment reads the old annotation and excludes `foo/releases/*`, but this current-only list writes `immutable-paths: []`. On the next unchanged deployment, history contains no retired path, so `sync --delete` removes those objects and still reports success. Carry the retained immutable history forward, with enough route history to preserve its key prefix, and cover the removal deployment plus the following deployment.
def render_site_manifests(site_name, action_dir, app_dir, manifests_dir, cfg): def render_site_manifests(
site_name, action_dir, app_dir, manifests_dir, cfg, previous_contracts=None,
):
"""Always re-render manifests from current site.yaml. Templates own """Always re-render manifests from current site.yaml. Templates own
Outdated
Review

Blocker: this persists only current immutable paths. Remove releases: deployment one preserves prior keys, then writes []; deployment two emits no retired filter and sync --delete removes them. Carry retired history forward and cover two deployments.

Blocker: this persists only current immutable paths. Remove `releases`: deployment one preserves prior keys, then writes `[]`; deployment two emits no retired filter and `sync --delete` removes them. Carry retired history forward and cover two deployments.
domain + aliases, so changes propagate without manual edits.""" domain + aliases, so changes propagate without manual edits."""
manifests_dir.mkdir(parents=True, exist_ok=True) manifests_dir.mkdir(parents=True, exist_ok=True)
artifact_by_name = {artifact["name"]: artifact for artifact in cfg["artifacts"]} artifact_by_name = {artifact["name"]: artifact for artifact in cfg["artifacts"]}
previous_contracts = previous_contracts or {}
routes = [] routes = []
for route in cfg["routes"]: for route in cfg["routes"]:
artifact = artifact_by_name[route["artifact"]] artifact = artifact_by_name[route["artifact"]]
resource_name = k8s_name(site_name) if cfg["compatibility"] else f"{k8s_name(site_name)}-{route['name']}" resource_name = k8s_name(site_name) if cfg["compatibility"] else f"{k8s_name(site_name)}-{route['name']}"
immutable_paths = [ previous = previous_contracts.get(artifact["bucket"])
rule["path"] for rule in artifact["cache_rules"] if _is_immutable(rule) immutable_paths = retained_immutable_paths(artifact, route, previous)
]
routes.append({ routes.append({
**route, "resource_name": resource_name, "artifact_config": artifact, **route, "resource_name": resource_name, "artifact_config": artifact,
"immutable_paths_json": json.dumps(immutable_paths, separators=(",", ":")), "immutable_paths_json": json.dumps(immutable_paths, separators=(",", ":")),
4
@@ -413,7 +426,9 @@ def deploy_static(site_name, site_dir, action_dir, token, cfg):
if cfg["compatibility"]: if cfg["compatibility"]:
ensure_bucket_aliases(site_name, cfg["aliases"], os.environ.get("GARAGE_ADMIN_TOKEN")) ensure_bucket_aliases(site_name, cfg["aliases"], os.environ.get("GARAGE_ADMIN_TOKEN"))
render_site_manifests(site_name, action_dir, app_dir, manifests_dir, cfg) render_site_manifests(
site_name, action_dir, app_dir, manifests_dir, cfg, previous_contracts,
)
commit_and_push(apps_dir, f"Deploy {site_name}", token) commit_and_push(apps_dir, f"Deploy {site_name}", token)
+39 -4
View File
@@ -473,7 +473,7 @@ class PublishingTests(unittest.TestCase):
): ):
deploy.s3_sync(artifact, route, root, previous_contract={ deploy.s3_sync(artifact, route, root, previous_contract={
"path": "/foo", "access": "public", "artifact": "distributions", "path": "/foo", "access": "public", "artifact": "distributions",
"immutable_paths": ["releases"], "immutable_paths": ["foo/releases"],
}) })
rendered = [" ".join(command) for command in commands] rendered = [" ".join(command) for command in commands]
self.assertTrue(all("foo/releases/*" in command for command in rendered[:2])) self.assertTrue(all("foo/releases/*" in command for command in rendered[:2]))
@@ -484,7 +484,7 @@ class PublishingTests(unittest.TestCase):
previous = { previous = {
"baseline-dist": { "baseline-dist": {
"path": "/dist", "access": "protected", "artifact": "old-name", "path": "/dist", "access": "protected", "artifact": "old-name",
"immutable_paths": ["releases"], "immutable_paths": ["dist/releases"],
} }
} }
with self.assertRaisesRegex(RuntimeError, "cannot become public while reusing protected"): with self.assertRaisesRegex(RuntimeError, "cannot become public while reusing protected"):
@@ -521,7 +521,7 @@ class PublishingTests(unittest.TestCase):
html = Path(tmp) html = Path(tmp)
filters = deploy.retired_immutable_filters(artifact, route, html, { filters = deploy.retired_immutable_filters(artifact, route, html, {
"path": "/dist", "access": "public", "artifact": "distributions", "path": "/dist", "access": "public", "artifact": "distributions",
"immutable_paths": ["releases"], "immutable_paths": ["dist/releases"],
}) })
self.assertEqual(["--exclude", "releases/*"], filters) self.assertEqual(["--exclude", "releases/*"], filters)
(html / "releases").mkdir() (html / "releases").mkdir()
@@ -529,9 +529,44 @@ class PublishingTests(unittest.TestCase):
with self.assertRaisesRegex(RuntimeError, "collides with retired immutable"): with self.assertRaisesRegex(RuntimeError, "collides with retired immutable"):
deploy.retired_immutable_filters(artifact, route, html, { deploy.retired_immutable_filters(artifact, route, html, {
"path": "/dist", "access": "public", "artifact": "distributions", "path": "/dist", "access": "public", "artifact": "distributions",
"immutable_paths": ["releases"], "immutable_paths": ["dist/releases"],
}) })
def test_retired_immutable_history_survives_the_following_deployment(self):
cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net")
artifact = next(item for item in cfg["artifacts"] if item["name"] == "distributions")
artifact["cache_rules"] = [
rule for rule in artifact["cache_rules"] if rule["path"] != "releases"
]
route = next(item for item in cfg["routes"] if item["artifact"] == "distributions")
route["path"] = "/"
previous = {
"baseline-dist": {
"path": "/foo", "access": "public", "artifact": "distributions",
"immutable_paths": ["foo/releases"],
}
}
with tempfile.TemporaryDirectory() as tmp:
root = Path(tmp)
app_dir = root / "app"
manifests = app_dir / "manifests"
app_dir.mkdir()
deploy.render_site_manifests(
"baseline.fritzlab.net", ROOT, app_dir, manifests, cfg, previous,
)
following = deploy.previous_route_contracts(app_dir)
self.assertEqual(["foo/releases"], following["baseline-dist"]["immutable_paths"])
html = root / "html"
html.mkdir()
first_filters = deploy.retired_immutable_filters(
artifact, route, html, previous["baseline-dist"],
)
following_filters = deploy.retired_immutable_filters(
artifact, route, html, following["baseline-dist"],
)
self.assertEqual(["--exclude", "foo/releases/*"], first_filters)
self.assertEqual(first_filters, following_filters)
def test_later_route_immutable_failure_stops_all_mutable_publication(self): def test_later_route_immutable_failure_stops_all_mutable_publication(self):
cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net") cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net")
with tempfile.TemporaryDirectory() as tmp: with tempfile.TemporaryDirectory() as tmp: