[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 69 additions and 25 deletions
Showing only changes of commit 9b0a8c4fd4 - Show all commits
+3 -3
View File
@@ -134,9 +134,9 @@ 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 and `site-publish-history.yaml` retain every Generated Ingress annotations and `site-publish-history.yaml` retain every
seen bucket's access, prior route, and cumulative bucket-relative immutable seen bucket's access, prior route, and cumulative bucket-relative immutable key
paths, including while an artifact is absent. When a move prefixes, including while an artifact is absent. Removed or renamed rules stay
places that retired prefix inside the new sync scope, only its declared recorded. When a move places a retired prefix inside the new sync scope, its
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.
+30 -21
View File
@@ -178,26 +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 recorded bucket keys when they fall inside the current sync 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("/")
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.
"/".join(part for part in (current_prefix, rule["path"]) if part)
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 = immutable_path if immutable_path in current_immutable:
continue
if current_prefix: if current_prefix:
marker = f"{current_prefix}/" marker = f"{current_prefix}/"
if not retired_path.startswith(marker): if not immutable_path.startswith(marker):
continue continue
retired_path = retired_path[len(marker):] 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}"
) )
@@ -305,8 +319,9 @@ 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(
previous_contracts=None): 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)
@@ -318,23 +333,17 @@ def render_site_manifests(site_name, action_dir, app_dir, manifests_dir, cfg,
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']}"
route_prefix = route["path"].strip("/")
immutable_paths = {
"/".join(part for part in (route_prefix, rule["path"]) if part)
for rule in artifact["cache_rules"] if _is_immutable(rule)
}
previous = previous_contracts.get(artifact["bucket"]) previous = previous_contracts.get(artifact["bucket"])
if previous: immutable_paths = retained_immutable_paths(artifact, route, previous)
immutable_paths.update(previous["immutable_paths"])
next_contracts[artifact["bucket"]] = { next_contracts[artifact["bucket"]] = {
"path": route["path"], "path": route["path"],
"access": "public" if route["access"] == "legacy" else route["access"], "access": "public" if route["access"] == "legacy" else route["access"],
"artifact": route["artifact"], "artifact": route["artifact"],
"immutable_paths": sorted(immutable_paths), "immutable_paths": immutable_paths,
} }
routes.append({ routes.append({
**route, "resource_name": resource_name, "artifact_config": artifact, **route, "resource_name": resource_name, "artifact_config": artifact,
Outdated
Review

Legacy routes normalize access to legacy, and this template persists that value. On the next deployment, this predicate rejects the action's own Ingress before any upload, stopping every legacy site. Accept legacy history or omit legacy annotations; add a two-deploy regression.

Legacy routes normalize access to `legacy`, and this template persists that value. On the next deployment, this predicate rejects the action's own Ingress before any upload, stopping every legacy site. Accept `legacy` history or omit legacy annotations; add a two-deploy regression.
"immutable_paths_json": json.dumps(sorted(immutable_paths), separators=(",", ":")), "immutable_paths_json": json.dumps(immutable_paths, separators=(",", ":")),
}) })
template_vars = { template_vars = {
"site": site_name, "site": site_name,
3
+36 -1
View File
@@ -485,7 +485,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"):
@@ -594,6 +594,41 @@ class PublishingTests(unittest.TestCase):
with self.assertRaisesRegex(RuntimeError, "invalid site-publish route history"): with self.assertRaisesRegex(RuntimeError, "invalid site-publish route history"):
deploy.previous_route_contracts(app_dir) deploy.previous_route_contracts(app_dir)
def test_retired_immutable_history_survives_a_route_move(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: