refactor(upload): resolve the upload id before the quarantine step

[max-trunk]

Stacked on #1196 — review that first. The base is max/print-test-case-guid-links, so this diff is the hoist alone.

What this changes

createBundleUpload ran inside upload_bundle, after the quarantine step had already logged each failure. So #1196 left one run printing two URL shapes for the same test: the report table on the GUID, the per-failure Learn more lines still on the short link.

This hoists the call above gather_exit_code_and_quarantined_tests_context and threads the resolved ids into log_failure, so both lanes agree. upload_bundle takes the resolved Result instead of making the call itself.

Read this before deciding it is worth landing

The user-visible effect is confined to --verbose. Those Learn more lines are tracing::info!, and cli/src/main.rs only installs the console layer when verbosity is above info — so in a default run they are not printed at all. A default run shows the stderr report table, which #1196 already put fully on the GUID form.

I originally justified this as fixing "two URL shapes in one run". That is real, but only under -v. Measured, not assumed — the new test drives --verbose because that is the only way the behaviour is observable, and it fails without this change.

So: reasonable to land for consistency and because it makes upload_bundle honest about not mutating meta, and reasonable to drop. Not much rides on it either way.

Why the reordering is safe

• The RPC error is still held until the upload below, where it has always surfaced. Failing at the new call site would skip quarantine resolution and change the exit code.
• The presigned S3 PUT URL has a 3600s TTL (METRICS_BUNDLE_UPLOADS_URL_TTL_SEC in trunk1's bundle_util.ts), so moving the quarantine fetch and internal-file generation ahead of the PUT has an hour of headroom. This was the objection I expected to find and did not.
• gather_upload_id_context mutates bundle_upload_id, bundle_upload_id_v2 and base_props.test_collection; nothing between the old and new call sites reads them, and upload_bundle no longer needs &mut BundleMeta at all.

Implementation notes

• Two request-order assertions (upload_bundle, test_variant_propagation) now see createBundleUpload before getQuarantineConfig. Updated in place — that reordering is the change, not an incidental break.
• Adds a 7th and 8th parameter to gather_exit_code_and_quarantined_tests_context and gather_quarantine_context, so both trip clippy::too_many_arguments. Left unsuppressed to match the two existing instances in the same file (9 and 8 params); happy to convert to a params struct if you would rather.

Testing

• trunk-analytics-cli --test upload: 59 passed, including the new --verbose case
• api: 25, --test test: 13, --test validate: 9
• Verified the new test fails on feat(upload): address failing tests by test-case GUID in collection links #1196's head — without the hoist, stdout carries the short link and not the GUID

🤖 Generated with Claude Code

[TylerJang27]

So this is calling CreateBundleUpload before GetQuarantineConfig? I really don't think we should be doing that. That messes with all the counts we have at upload time for failed tests/quarantined tests/etc

[max-trunk]

I'm still reviewing these two PRs, was going to scrutinize this one in particular to see if it was actually necessary

[max-trunk]

Closing this — the reordering costs more than the inconsistency it fixes.

Why not land it

The problem is smaller than I first described. The per-failure Learn more lines are tracing::info!, and cli/src/main.rs only installs the console layer above info level, so they are not printed at all in a default run. #1196 already puts the default output — the stderr report table — fully on the GUID form. What this PR fixes is a two-URL-shape inconsistency visible only under --verbose.

And moving the call perturbs two measurements. upload_started_at is stamped at upload_command.rs:607, after where this PR hoists createBundleUpload. Today that window spans RPC + tarball + S3 PUT; after the hoist it spans only tarball + S3 PUT, so client-reported upload duration steps down with nothing on the trunk1 side to explain it. Server-side, the receipt's bundle_meta_created_at is stamped at createBundleUpload, so hoisting widens the receipt→S3-object window by the quarantine fetch plus internal-file generation — inflating that latency and lengthening the window in which a dying process leaves an orphaned receipt.

Neither is a correctness bug. Both are the kind of thing that shows up as an unexplained step in a graph weeks later, which is a bad trade for a --verbose-only cosmetic fix.

The better shape, if this ever matters

Return repo_id and test_collection_id on /metrics/getQuarantineConfig as well, and have log_failure read them from there. It runs immediately after that response arrives, so both lanes get the GUID with no call reordering and no metric shift.

Groundwork is mostly there: repo_id is already field 2 on GetQuarantinedTestIdsRequest (public-api resolves it before the gRPC hop, so echoing it back is free), testCollectionId is already resolved as CollectionQuarantineSettings.testCollectionId, and the response message has 5 fields so 6 and 7 are additive.

One thing to fix at the same time: testCollectionId is only resolved on the collection lane. An org that resolves quarantine from the repo — not fully migrated, collection quarantining not explicitly enabled — never has one resolved even when a short id was passed, while ingest still writes a GUID for those uploads. Sourcing from this endpoint as-is would therefore yield fewer GUIDs than createBundleUpload does, and the gap is widest during the current rollout.

The likely trigger for revisiting: promoting the Learn more lines out of verbose-only, which is a defensible change on its own.

The branch max/hoist-create-bundle-upload is left in place if anyone wants the diff.

🤖 Generated with Claude Code