[HOLD] Explicit and unique ownership of HTTPS Transaction objects.

[tylerkaraszewski]

Details

See full explanation in description here:
https://github.com/Expensify/Auth/pull/19955

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/599836
$ https://github.com/Expensify/Expensify/issues/583441

Tests

Existing tests.

---

Internal Testing Reminder: when changing bedrock, please compile auth against your new changes

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a8396e45d9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[tylerkaraszewski]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[nkuoch]

PR #2515 (Expensify/Bedrock) – Review

Summary

The PR refactors HTTPS transaction ownership from raw pointers to unique_ptr: BedrockCommand::httpsRequests becomes GrowOnlyList<unique_ptr<SHTTPSManager::Transaction>>, and SHTTPSManager’s API is updated to use unique_ptr and references. This improves ownership semantics and removes manual closeTransaction/remove handling.

---

Critical: Breaking API change for Auth (and other plugins)

SHTTPSManager (libstuff) is a shared API used by Auth and other Bedrock plugins. This PR changes that API in a breaking way:

Change	Impact on Auth
_httpsSend() returns unique_ptr<Transaction> instead of Transaction*	Auth overrides in Hubspot, Stripe, TestHubspot, and all _httpsSend call sites must return and use unique_ptr.
_onRecv(Transaction* t) → _onRecv(Transaction& t)	Auth overrides in AuthCommandManager, Stripe, TestStripe, AuthScraper, XeroSSOIdentityAPI, ITunesBilling, Google/AppleSSOVerification, MockSHTTPSManager, etc. must be updated to take Transaction&.
closeTransaction(Transaction*) removed	Auth does not call it; no change needed.
httpsRequests is list<unique_ptr<Transaction>>	All Auth code that does httpsRequests.push_back(...) with a raw pointer (dozens of call sites in SendDataToHubspot, GetVirtualCard, MigrateWorkspaceToCustomObjectForHubspot, Card, Hubspot, ProcessAgentZeroRequest, etc.) must be updated to push_back(move(unique_ptr)).

Recommendation: Before or with merging this PR:

1. Either have a coordinated Auth (and any other plugin) PR that updates to the new API, or
2. Clearly document in the PR description that merging this will break Auth until a follow-up Auth PR is merged, and ensure that follow-up is planned and tracked.

Without that, the Bedrock + Auth build will break after merge.

---

Bedrock-side correctness

• Ownership and cleanup

  • BedrockCommand no longer needs to call closeTransaction in the destructor; the list of unique_ptrs is destroyed with the command and each transaction (and its socket) is cleaned up in Transaction::~Transaction() via delete s.
  • In _httpsSend, on exception the local unique_ptr is destroyed on return, so the previous delete transaction is correctly replaced by RAII.

• startFunc

  • Signature change from void(Transaction*) to void(Transaction&) and call site (transaction->startFunc)(*transaction) are consistent.

• GrowOnlyList

  • Removing push_back(const T&) and keeping only push_back(T&&) is correct for T = unique_ptr<...>.
  • front()/back() return type changed from auto to const T& is clearer and still correct.

• SSLTest

  • Dropping the explicit manager.closeTransaction(transaction) is correct; the unique_ptr destroys the transaction when it goes out of scope.

• TestPlugin httpstimeout test

  • Capturing a raw pointer for the detached thread and documenting that the test assumes the command outlives the thread is a reasonable approach for this test; the comment is clear.

---

Minor / style

• TestPlugin.cpp

  • The comment says “this line and the thread we pass it to unsafely accesses this memory” — consider “unsafely access” for subject–verb agreement.

• Included headers

  • #include <memory> added in TestPlugin.cpp for make_unique/unique_ptr is appropriate.

---

Summary table

Area	Verdict
Bedrock refactor (ownership, destructors, API usage)	Looks correct
Test updates (TestPlugin, TestHTTPS, SSLTest)	Consistent with new API
Breaking change for Auth/plugins	Must be called out and handled (coordinated or follow-up PR)

Verdict: The refactor inside Bedrock is sound. Approval should be conditional on either (1) a coordinated Auth (and plugin) update, or (2) an explicit plan and ticket for an Auth follow-up PR, so the breakage is not merged without a path to fix.