ReviewOS

also looking at this

pantry-pm/pantry

fix(publish): an accepted npm upload no longer fails on a slow readback

#259
Merged glennmichael123 wants to merge fix/245-npm-publish-confirm into main
3 files +117 -11
packages/zig/src/cli/commands/package.zigmodified+96-11
Changes to packages/zig/src/cli/commands/package.zig
@@ -778,6 +778,10 @@ pub const PublishOptions = struct {
778778 /// stx release spent 100 to 120 seconds on each of its packages in turn,
779779 /// three stages of ten minutes - and queued, those delays overlap.
780780 deferred_readbacks: ?*ReadbackQueue = null,
781 /// How long to wait for an accepted upload to become public before
782 /// reporting it pending, in milliseconds (`--confirm-timeout`, seconds).
783 /// Null uses `npm_readback_timeout_ms`.
784 confirm_timeout_ms: ?u64 = null,
781785};
782786
783787pub const DeferredReadback = publish_plan.DeferredReadback;
@@ -1118,6 +1122,34 @@ test "a bounded timeout before sending an upload is retryable" {
11181122 try std.testing.expect(isTransientNetworkError(error.Timeout));
11191123}
11201124
1125test "the public readback has its own window, not the upload socket's" {
1126 // The regression this pins: both were `npm_publish_timeout_ms`, so a
1127 // readback gave up after the time allotted to a stalled PUT and reported
1128 // a version that did go live as a failure (#245).
1129 try std.testing.expect(npm_readback_timeout_ms != npm_publish_timeout_ms);
1130 try std.testing.expect(npm_readback_timeout_ms > npm_publish_timeout_ms);
1131
1132 // Default, and --confirm-timeout overriding it.
1133 try std.testing.expectEqual(npm_readback_timeout_ms, readbackTimeoutMs(.{}));
1134 try std.testing.expectEqual(@as(u64, 1_800_000), readbackTimeoutMs(.{ .confirm_timeout_ms = 1_800_000 }));
1135}
1136
1137test "an accepted upload completes even when npm has not shown it yet" {
1138 const allocator = std.testing.allocator;
1139
1140 // npm accepted the bytes and the readback timed out. That used to be
1141 // exit 1, which skipped the release binaries and the GitHub release for a
1142 // version that went live a minute later — and the message forbids
1143 // re-uploading, so there was no safe way to re-run (#245).
1144 var pending = completeNpmPublication(allocator, "demo", "1.0.0", null, ".", .{}, false);
1145 defer pending.deinit(allocator);
1146 try std.testing.expectEqual(@as(u8, 0), pending.exit_code);
1147
1148 var confirmed = completeNpmPublication(allocator, "demo", "1.0.0", null, ".", .{}, true);
1149 defer confirmed.deinit(allocator);
1150 try std.testing.expectEqual(@as(u8, 0), confirmed.exit_code);
1151}
1152
11211153/// Reason a package should be skipped when publishing to npm, or null to publish.
11221154///
11231155/// Native (Zig) packages live on the Pantry registry, not npm: their source is
@@ -1158,9 +1190,45 @@ fn completeVerifiedNpmPublication(
11581190 scripts: ?std.json.ObjectMap,
11591191 package_dir: []const u8,
11601192 options: PublishOptions,
1193) CommandResult {
1194 return completeNpmPublication(allocator, name, version, scripts, package_dir, options, true);
1195}
1196
1197/// Finish an upload npm accepted, whether or not it is public yet.
1198///
1199/// A readback that times out used to return exit 1. But npm had accepted the
1200/// bytes: the version lands, and the message says not to upload it again — so
1201/// the only effect was to skip every release step after the publish (binaries,
1202/// GitHub release) for a version that went live a minute later, with no safe
1203/// way to re-run (#245). An accepted upload therefore completes, and the
1204/// pending line says what is still outstanding.
1205///
1206/// This applies ONLY where npm accepted this run's upload. An upload whose
1207/// outcome is unknown — a socket that died after the bytes, a 5xx or a
1208/// redirect over a committed write — still fails, because there the version
1209/// may never appear and a release built on it would reference nothing.
1210///
1211/// A mismatch is not pending either: `confirmPublication` returns
1212/// `error.PublicationIntegrityMismatch` rather than false when the public
1213/// tarball is not the one we sent, and that error propagates to a failure.
1214/// What reaches `confirmed = false` is only "npm has not shown it yet".
1215fn completeNpmPublication(
1216 allocator: std.mem.Allocator,
1217 name: []const u8,
1218 version: []const u8,
1219 scripts: ?std.json.ObjectMap,
1220 package_dir: []const u8,
1221 options: PublishOptions,
1222 confirmed: bool,
11611223) CommandResult {
11621224 const queued = if (options.deferred_readbacks) |queue| queue.items.len > 0 and std.mem.eql(u8, queue.items[queue.items.len - 1].name, name) and std.mem.eql(u8, queue.items[queue.items.len - 1].version, version) else false;
1163 style.print("\n✓ {s} {s}@{s}\n", .{ if (queued) "Uploaded" else "Published", name, version });
1225 if (!confirmed) {
1226 // The same text release tooling already recognises, kept verbatim and
1227 // still printed under this package's receipt — as a warning now, not
1228 // an error, because the publish itself succeeded.
1229 style.print(" {s}⚠{s} {s}\n", .{ style.yellow, style.reset, publish_plan.pending_publication });
1230 }
1231 style.print("\n✓ {s} {s}@{s}\n", .{ if (queued or !confirmed) "Uploaded" else "Published", name, version });
11641232 if (!options.ignore_scripts) {
11651233 if (scripts) |configured| {
11661234 _ = lib.lifecycle.runPostPublishScripts(allocator, configured, package_dir);
@@ -1247,7 +1315,7 @@ fn publishSingleToNpm(
12471315 .message = try std.fmt.allocPrint(allocator, "Public version availability could not be verified: {s}. Upload was not started.", .{@errorName(err)}),
12481316 };
12491317 if (exists) {
1250 if (!try precheck_client.confirmPublication(metadata.name, metadata.version, null, npm_publish_timeout_ms)) {
1318 if (!try precheck_client.confirmPublication(metadata.name, metadata.version, null, readbackTimeoutMs(options))) {
12511319 return CommandResult.err(allocator, "Existing version is pending public tarball availability. Do not upload this version again.");
12521320 }
12531321 style.print("\n{s}↷{s} {s}@{s} already published — skipping\n", .{ style.dim, style.reset, metadata.name, metadata.version });
@@ -1376,7 +1444,7 @@ fn publishSingleToNpm(
13761444 const deferred = options.readback and options.deferred_readbacks != null;
13771445 if (deferred) try deferReadback(allocator, options.deferred_readbacks.?, metadata.name, metadata.version, tarball_info.shasum, tarball_info.integrity, registry_url);
13781446 const confirm_started = io_helper.getMilliTimestamp();
1379 const confirmed = !options.readback or deferred or try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, npm_publish_timeout_ms);
1447 const confirmed = !options.readback or deferred or try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, readbackTimeoutMs(options));
13801448 const confirm_ms = io_helper.getMilliTimestamp() - confirm_started;
13811449 if (deferred)
13821450 style.print(" {s}accepted by npm; read back once a package needs it or the batch is uploaded{s}\n", .{ style.dim, style.reset })
@@ -1385,7 +1453,7 @@ fn publishSingleToNpm(
13851453 else
13861454 style.print(" {s}accepted by npm; public readback left to the caller (--no-readback){s}\n", .{ style.dim, style.reset });
13871455 if (!confirmed) {
1388 return CommandResult.err(allocator, publish_plan.pending_publication);
1456 return completeNpmPublication(allocator, metadata.name, metadata.version, scripts_obj, package_dir, options, false);
13891457 }
13901458 // A queued readback reports "Published" when it confirms.
13911459 const done = if (deferred) "Uploaded" else "Published";
@@ -1420,7 +1488,7 @@ fn publishSingleToNpm(
14201488 // for it contradicts the pre-check and, mid-release, strands
14211489 // the packages that already went out.
14221490 if (options.skip_existing) {
1423 if (!try registry_client.confirmPublication(metadata.name, metadata.version, null, npm_publish_timeout_ms)) {
1491 if (!try registry_client.confirmPublication(metadata.name, metadata.version, null, readbackTimeoutMs(options))) {
14241492 return CommandResult.err(allocator, "Claimed version is pending public metadata and tarball availability. Do not upload this version again.");
14251493 }
14261494 style.print("\n{s}↷{s} {s}@{s} already claimed on npm — skipping\n", .{ style.dim, style.reset, metadata.name, metadata.version });
@@ -1461,7 +1529,7 @@ fn publishSingleToNpm(
14611529 // before the upload — no CI provider, a token npm would not
14621530 // exchange for a package with no trusted publisher — which used to
14631531 // wait two minutes per package for a version that was never sent.
1464 if (result.sent and try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, npm_publish_timeout_ms)) {
1532 if (result.sent and try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, readbackTimeoutMs(options))) {
14651533 style.print(
14661534 "\n{s}✓{s} {s}@{s} is on the registry — the publish landed, the response did not\n",
14671535 .{ style.green, style.reset, metadata.name, metadata.version },
@@ -1557,7 +1625,7 @@ fn publishSingleToNpm(
15571625 // A request can commit before its response fails. Never send a
15581626 // second PUT after bytes may have reached the registry.
15591627 if (registry_client.token_upload_started) {
1560 if (try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, npm_publish_timeout_ms)) {
1628 if (try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, readbackTimeoutMs(options))) {
15611629 return completeVerifiedNpmPublication(allocator, metadata.name, metadata.version, scripts_obj, package_dir, options);
15621630 }
15631631 return CommandResult.err(allocator, "Upload outcome is pending public verification. Do not send another upload while this version may be staged.");
@@ -1596,7 +1664,7 @@ fn publishSingleToNpm(
15961664 if (r.status_code == 408 or (r.status_code >= 500 and r.status_code < 600) or (r.status_code >= 300 and r.status_code < 400)) {
15971665 var owned_response = r;
15981666 defer owned_response.deinit(allocator);
1599 if (try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, npm_publish_timeout_ms)) {
1667 if (try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, readbackTimeoutMs(options))) {
16001668 return completeVerifiedNpmPublication(allocator, metadata.name, metadata.version, scripts_obj, package_dir, options);
16011669 }
16021670 return CommandResult.err(allocator, "Upload outcome is pending public verification. Do not send another upload while this version may be staged.");
@@ -1636,8 +1704,8 @@ fn publishSingleToNpm(
16361704 if (options.readback and options.deferred_readbacks != null) {
16371705 try deferReadback(allocator, options.deferred_readbacks.?, metadata.name, metadata.version, tarball_info.shasum, tarball_info.integrity, registry_url);
16381706 style.print(" {s}accepted by npm; read back once a package needs it or the batch is uploaded{s}\n", .{ style.dim, style.reset });
1639 } else if (options.readback and !try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, npm_publish_timeout_ms)) {
1640 return CommandResult.err(allocator, publish_plan.pending_publication);
1707 } else if (options.readback and !try registry_client.confirmPublication(metadata.name, metadata.version, &tarball_info.integrity, readbackTimeoutMs(options))) {
1708 return completeNpmPublication(allocator, metadata.name, metadata.version, scripts_obj, package_dir, options, false);
16411709 }
16421710 return completeVerifiedNpmPublication(allocator, metadata.name, metadata.version, scripts_obj, package_dir, options);
16431711 } else {
@@ -1662,7 +1730,7 @@ fn publishSingleToNpm(
16621730 // See the OIDC path above: under `skip_existing` a conflict is the
16631731 // pre-check losing a race, not a mistake worth failing over.
16641732 if (options.skip_existing) {
1665 if (!try registry_client.confirmPublication(metadata.name, metadata.version, null, npm_publish_timeout_ms)) {
1733 if (!try registry_client.confirmPublication(metadata.name, metadata.version, null, readbackTimeoutMs(options))) {
16661734 return CommandResult.err(allocator, "Claimed version is pending public metadata and tarball availability. Do not upload this version again.");
16671735 }
16681736 const why: []const u8 = if (conflict_kind == .staged)
@@ -1901,6 +1969,23 @@ const npm_publish_timeout_ms: u64 = 120_000;
19011969/// that the upload should be sent again.
19021970const deferred_readback_timeout_ms: u64 = 600_000;
19031971
1972/// How long an immediate readback waits for npm to make an accepted upload
1973/// public. Separate from `npm_publish_timeout_ms`, which bounds the upload
1974/// socket: the two were the same value, so a readback gave up after the time
1975/// allotted to a stalled PUT. npm's propagation regularly passes two minutes
1976/// and has been observed around 25 minutes on a Craft release, which is what
1977/// made `publish --npm` exit 1 on a version that did go live and skip every
1978/// release step after it (#245). The batch path had already been raised to
1979/// ten minutes for the same reason; this matches it. `--confirm-timeout`
1980/// overrides it where a publisher knows its registry is slower still.
1981const npm_readback_timeout_ms: u64 = 600_000;
1982
1983/// The readback window for this publish: `--confirm-timeout`, else the
1984/// default above.
1985fn readbackTimeoutMs(options: PublishOptions) u64 {
1986 return options.confirm_timeout_ms orelse npm_readback_timeout_ms;
1987}
1988
19041989const OIDCPublishTaskResult = anyerror!OIDCPublishResult;
19051990
19061991fn attemptOIDCPublishTask(
packages/zig/src/lib.zigmodified+6-0
Changes to packages/zig/src/lib.zig
@@ -70,6 +70,12 @@ test {
7070 _ = ;
7171 _ = ;
7272 _ = ;
73 // package.zig's own tests were never reached: refAllDecls does not pull
74 // them in and nothing imported it for its own sake, so its five existing
75 // tests had been dead since they were written. This is why publish_plan.zig
76 // and publish_conflict.zig exist as separate files — testable logic was
77 // moved out of package.zig rather than making its tests run.
78 _ = ;
7379 _ = ;
7480 _ = ;
7581 _ = ;
packages/zig/src/main.zigmodified+15-0
Changes to packages/zig/src/main.zig
@@ -899,6 +899,17 @@ fn whoamiAction(ctx: *cli.BaseCommand.ParseContext) !void {
899899 std.process.exit(result.exit_code);
900900}
901901
902/// `--confirm-timeout <seconds>` as milliseconds, or null for the default.
903///
904/// Seconds at the command line because the thing being waited on is npm's
905/// propagation delay, which publishers talk about in minutes.
906fn confirmTimeoutMs(ctx: *cli.BaseCommand.ParseContext) ?u64 {
907 const raw = ctx.getOption("confirm-timeout") orelse return null;
908 const seconds = std.fmt.parseInt(u64, std.mem.trim(u8, raw, " \t"), 10) catch return null;
909 if (seconds == 0) return null;
910 return seconds * 1000;
911}
912
902913fn publishAction(ctx: *cli.BaseCommand.ParseContext) !void {
903914 const allocator = ctx.allocator;
904915
@@ -929,6 +940,7 @@ fn publishAction(ctx: *cli.BaseCommand.ParseContext) !void {
929940 .skip_existing = !force_republish,
930941 .ignore_scripts = ignore_scripts,
931942 .readback = !ctx.hasOption("no-readback"),
943 .confirm_timeout_ms = confirmTimeoutMs(ctx),
932944 };
933945
934946 const result = try lib.commands.publishCommand(allocator, &[_][]const u8{}, options);
@@ -1428,6 +1440,7 @@ fn registryPublishAction(ctx: *cli.BaseCommand.ParseContext) !void {
14281440 .skip_existing = !force_republish,
14291441 .ignore_scripts = ignore_scripts,
14301442 .readback = !ctx.hasOption("no-readback"),
1443 .confirm_timeout_ms = confirmTimeoutMs(ctx),
14311444 };
14321445
14331446 const result = try lib.commands.publishCommand(allocator, paths.items, npm_options);
@@ -3624,6 +3637,7 @@ pub fn main() !void {
36243637 _ = try npm_publish_cmd.addOption(npm_force_republish_opt);
36253638 const npm_no_readback_opt = cli.Option.init("no-readback", "no-readback", "Report an accepted upload without reading it back from the public registry (for a caller that verifies every package itself)", .bool);
36263639 _ = try npm_publish_cmd.addOption(npm_no_readback_opt);
3640 _ = try npm_publish_cmd.addOption(cli.Option.init("confirm-timeout", "confirm-timeout", "Seconds to wait for an accepted upload to become public before reporting it pending (default 600)", .string));
36273641
36283642 const npm_ignore_scripts_opt = cli.Option.init("ignore-scripts", "ignore-scripts", "Don't run package lifecycle scripts (prepublishOnly/prepack/etc.); use with a separate build step", .bool);
36293643 _ = try npm_publish_cmd.addOption(npm_ignore_scripts_opt);
@@ -4711,6 +4725,7 @@ pub fn main() !void {
47114725 _ = try publish_cmd.addOption(pub_ignore_scripts_opt);
47124726 const pub_no_readback_opt = cli.Option.init("no-readback", "no-readback", "Report an accepted npm upload without reading it back from the public registry (for a caller that verifies every package itself)", .bool);
47134727 _ = try publish_cmd.addOption(pub_no_readback_opt);
4728 _ = try publish_cmd.addOption(cli.Option.init("confirm-timeout", "confirm-timeout", "Seconds to wait for an accepted npm upload to become public before reporting it pending (default 600)", .string));
47144729
47154730 const pub_paths_arg = cli.Argument.init("paths", "Package directories or globs to publish (e.g., './packages/*'); defaults to auto-detecting packages/", .string)
47164731 .withRequired(false)