Skip to content

fix(signingscript): Bug 2034166 reduce notarization requests #1426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
hneiva wants to merge 1 commit into
base: master
Choose a base branch
from
+160 −31
Draft

fix(signingscript): Bug 2034166 reduce notarization requests #1426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 66 additions & 29 deletions signingscript/src/signingscript/sign.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1706,13 +1706,50 @@ async def sign_rpm_pkg(context, path, fmt, **kwargs):
return path


async def _notarize_wait_staple_batch(context, paths, attempts):
"""Serial notarize-submit, notary-wait, then staple for a list of paths.

No-op on empty list. Each step wraps the rcodesign call in retry_async
with the given attempt budget, raising RCodesignError on exhaustion.
"""
if not paths:
return
submissions_map = {}
for path in paths:
submissions_map[path] = await retry_async(
func=rcodesign_notarize,
args=(path, context.apple_credentials_path),
attempts=attempts,
retry_exceptions=RCodesignError,
)
for path, submission_id in submissions_map.items():
await retry_async(
func=rcodesign_notary_wait,
args=(submission_id, context.apple_credentials_path),
attempts=attempts,
retry_exceptions=RCodesignError,
)
for path in submissions_map.keys():
await retry_async(
func=rcodesign_staple,
args=[path],
attempts=attempts,
retry_exceptions=RCodesignError,
)


@time_async_function
async def apple_notarize_stacked(context, filelist_dict):
"""
Notarizes multiple packages using rcodesign.
Submits everything before polling for status.

Fully notarizes and staples .pkg paths first, then probes each .app with
a short-retry staple attempt. .apps that fail the probe fall back to the
full notarize/wait/staple pipeline. This avoids redundant notarization
requests for .apps that become valid transitively via their parent .pkg.
"""
ATTEMPTS = 5
STAPLE_PROBE_RETRY_KWARGS = {"attempts": 3, "sleeptime_kwargs": {"delay_factor": 15}}

relpath_index_map = {}
paths_to_notarize = []
Expand Down Expand Up @@ -1742,34 +1779,34 @@ async def apple_notarize_stacked(context, filelist_dict):
else:
raise SigningScriptError(f"Unsupported file extension: {extension} for file {relpath}")

# notarization submissions map (path -> submission_id)
submissions_map = {}
# Submit to notarization one by one
for path in paths_to_notarize:
submissions_map[path] = await retry_async(
func=rcodesign_notarize,
args=(path, context.apple_credentials_path),
attempts=ATTEMPTS,
retry_exceptions=RCodesignError,
)

# Notary wait all files
for path, submission_id in submissions_map.items():
await retry_async(
func=rcodesign_notary_wait,
args=(submission_id, context.apple_credentials_path),
attempts=ATTEMPTS,
retry_exceptions=RCodesignError,
)

# Staple files
for path in submissions_map.keys():
await retry_async(
func=rcodesign_staple,
args=[path],
attempts=ATTEMPTS,
retry_exceptions=RCodesignError,
)
pkg_paths = [p for p in paths_to_notarize if p.endswith(".pkg")]
app_paths = [p for p in paths_to_notarize if p.endswith(".app")]

# Phase A: full notarize/wait/staple pipeline for every .pkg
await _notarize_wait_staple_batch(context, pkg_paths, ATTEMPTS)

# Phase B: staple probe per .app; success means the .app was transitively
# validated by its parent .pkg in Phase A. When no .pkg ran in Phase A,
# there's no parent ticket to wait for, so probe just once (no retry).
# The single-attempt probe also covers the split-task case: .apps and
# .pkgs can be notarized in separate tasks with an app->pkg dependency
# in CI, so by the time the .app task runs its parent .pkg is already
# stapled and the probe succeeds on the first try.
probe_kwargs = STAPLE_PROBE_RETRY_KWARGS if pkg_paths else {"attempts": 1}
Copy link
Copy Markdown
Contributor

@bhearsum bhearsum Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why this is as it is, but this override based on pkg_paths is a code smell. Even for cases where we have both .pkg and .app inputs to notarize, this breaks down if an .app is not contained within a .pkg.

Rather than relying on retries, can we do something a bit more intelligent here? Perhaps we can keep track of which .app packages are in a .pkg and do the right thing based on that instead?

(This issue also goes away if we rework the order things are done in CI, of course.)

Copy link
Copy Markdown
Contributor Author

@hneiva hneiva Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmmm that's a good point.

Unfortunately I don't know of an easy way of checking what's inside the pkg without having to unpack the whole thing just to find out which locale is it.

I think the best long-term solution is to:

It also begs the question, should we always staple probe before sending for notarization? (even on pkgs) Reruns on chunked tasks are wasteful, since they always submit everything to notarization even if previous attempts have already notarized.
Or maybe this task should be "run-aware"? If it's the first run, don't probe pkgs?

In an ideal world I think we'd adjust the CI flow to go "sign .app" -> "notarize .app" -> "create .pkg" -> "sign .pkg" -> "notarize .pkg"

I think we should avoid 2 notarizations here.
What we can do instead (the patch I liked gets us close to this):
sign .app -> create .pkg -> notarize+staple .pkg -> only staple the .app (which becomes the .dmg)

The .app inside the .pkg doesn't have to be notarized before it's packed inside the pkg.

Copy link
Copy Markdown
Contributor

@bhearsum bhearsum Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I don't know of an easy way of checking what's inside the pkg without having to unpack the whole thing just to find out which locale is it.

Fair enough -- and unpacking just for this is probably expensive enough that it may not be worthwhile.

It also begs the question, should we always staple probe before sending for notarization? (even on pkgs) Reruns on chunked tasks are wasteful, since they always submit everything to notarization even if previous attempts have already notarized.

When you say "staple probe", do you mean inspection of local artifacts, or querying the remote server? If it's the latter, I think that's an excellent idea (more on that below).

Or maybe this task should be "run-aware"? If it's the first run, don't probe pkgs?

This is an excellent idea that might be good to take up later/elsewhere. On runs > 0 we could have no, some, or all packages already notarized...and ideally we'd handle that gracefully. We do a similar thing in landoscript where we dump out status when submitting a job, but before polling its status. And when we first start up, we look for status from an earlier run when run_id is > 0.

In the case of notarization, I think there's one or more URLs we'd want to write out and use in a similar fashion?

sign .app -> create .pkg -> notarize+staple .pkg -> only staple the .app (which becomes the .dmg)

The .app inside the .pkg doesn't have to be notarized before it's packed inside the pkg.

Are we sure this doesn't have negative consequences? Presumably the .app still has its notarization status checked (whether through talking to apple, or checking for a stapled ticket)?

Or does it get implicitly notarized when the pkg is submitted? If that's the case, my view is that it's more comprehensible to sign and notarize the .app first, and then use the notarized app to make the pkg. The idea of making the app, then stuffing it into a pkg, then notarizing that, then pulling the app out of it and stapling it afterwards seems awkward to me. It also seems like it risks running into update issues -- if the .app in the pkg and the dmg are not identical, partial mars will fail for one or the other.

I'd be happy to hop on Zoom to hash this out more at some point if that's useful; we might be reaching the limits of what we can hash out here.

In any case, if there's plans to improve and rework this in the near future I'm a little less fussed about what's happening here if it gives us a short term win.

apps_needing_notarization = []
for app_path in app_paths:
try:
await retry_async(
func=rcodesign_staple,
args=[app_path],
retry_exceptions=RCodesignError,
**probe_kwargs,
)
except RCodesignError:
apps_needing_notarization.append(app_path)

# Phase C: full pipeline fallback for .apps that failed the probe
await _notarize_wait_staple_batch(context, apps_needing_notarization, ATTEMPTS)

# Wrap up
stapled_files = []
Expand Down
96 changes: 94 additions & 2 deletions signingscript/tests/test_sign.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1487,10 +1487,102 @@ async def test_apple_notarize_stacked(mocker, context):
"/app2.pkg": {"full_path": "/app2.pkg", "formats": ["apple_notarize_stacked"]},
},
)
# one for each file format
# Phase A notarizes/waits the 2 .pkgs; the 1 .app is transitively validated
# by its parent .pkg, so only its Phase B staple probe runs (no notarize/wait).
assert notarize.await_count == 2
assert wait.await_count == 2
assert staple.await_count == 3


@pytest.mark.asyncio
async def test_apple_notarize_stacked_probe_fallback(mocker, context):
""".app staple probe fails -> fall back to full notarize/wait/staple."""

async def no_retry(func=None, args=(), kwargs=None, attempts=1, retry_exceptions=Exception, **_):
kwargs = kwargs or {}
return await func(*args, **kwargs)

mocker.patch.object(sign, "retry_async", new=no_retry)

notarize = mock.AsyncMock()
mocker.patch.object(sign, "rcodesign_notarize", notarize)
wait = mock.AsyncMock()
mocker.patch.object(sign, "rcodesign_notary_wait", wait)

app_probe_failures = {"remaining": 1}

async def staple_side_effect(path):
if path.endswith(".app") and app_probe_failures["remaining"] > 0:
app_probe_failures["remaining"] -= 1
raise sign.RCodesignError("simulated probe failure")
return None

staple = mock.AsyncMock(side_effect=staple_side_effect)
mocker.patch.object(sign, "rcodesign_staple", staple)

mocker.patch.object(sign, "_extract_tarfile", noop_async)
mocker.patch.object(sign, "_create_tarfile", noop_async)
mocker.patch.object(sign.os, "listdir", lambda *_: ["/foo.pkg", "/baz.app", "/foobar"])
mocker.patch.object(sign.os, "walk", lambda *_: [("/", None, ["foo.pkg", "baz.app"])])
mocker.patch.object(sign.shutil, "rmtree", noop_sync)
mocker.patch.object(sign.utils, "mkdir", noop_sync)
mocker.patch.object(sign.utils, "copy_to_dir", noop_sync)

await sign.apple_notarize_stacked(
context,
{
"/app.tar.gz": {"full_path": "/app.tar.gz", "formats": ["apple_notarize_stacked"]},
"/app2.pkg": {"full_path": "/app2.pkg", "formats": ["apple_notarize_stacked"]},
},
)
# Phase A: 2 .pkgs notarized + waited + stapled.
# Phase B: 1 probe attempt on the .app (raises).
# Phase C: fallback notarize + wait + staple for that .app.
assert notarize.await_count == 3
assert wait.await_count == 3
assert staple.await_count == 3
assert staple.await_count == 4
fallback_notarize = [c for c in notarize.await_args_list if c.args[0].endswith(".app")]
assert len(fallback_notarize) == 1


@pytest.mark.asyncio
async def test_apple_notarize_stacked_no_pkg_single_probe(mocker, context):
"""When no .pkg is in the batch, .apps get a single-attempt probe, then Phase C."""
notarize = mock.AsyncMock()
mocker.patch.object(sign, "rcodesign_notarize", notarize)
wait = mock.AsyncMock()
mocker.patch.object(sign, "rcodesign_notary_wait", wait)

# Probe fails once; fallback staple in Phase C succeeds.
app_probe_failures = {"remaining": 1}

async def staple_side_effect(path):
if path.endswith(".app") and app_probe_failures["remaining"] > 0:
app_probe_failures["remaining"] -= 1
raise sign.RCodesignError("simulated probe failure")
return None

staple = mock.AsyncMock(side_effect=staple_side_effect)
mocker.patch.object(sign, "rcodesign_staple", staple)

mocker.patch.object(sign, "_extract_tarfile", noop_async)
mocker.patch.object(sign, "_create_tarfile", noop_async)
# tar.gz extracts to a .app only (no .pkg alongside)
mocker.patch.object(sign.os, "listdir", lambda *_: ["/baz.app", "/foobar"])
mocker.patch.object(sign.os, "walk", lambda *_: [("/", None, ["baz.app"])])
mocker.patch.object(sign.shutil, "rmtree", noop_sync)
mocker.patch.object(sign.utils, "mkdir", noop_sync)
mocker.patch.object(sign.utils, "copy_to_dir", noop_sync)

await sign.apple_notarize_stacked(
context,
{"/app.tar.gz": {"full_path": "/app.tar.gz", "formats": ["apple_notarize_stacked"]}},
)
# No .pkgs -> Phase A empty. Phase B probes once (fails, no retry).
# Phase C notarizes/waits/staples the .app.
assert notarize.await_count == 1
assert wait.await_count == 1
assert staple.await_count == 2 # 1 probe (raises) + 1 Phase C staple (succeeds)


@pytest.mark.asyncio
Expand Down