feat: Make settings not thread-local #1444

scouten-adobe · 2025-09-24T05:12:23Z

Settings are now snapshotted at public API surfaces and passed through all layers below.

Note related work from @ok-nick in #1447 and further work likely in #1454.

codecov · 2025-09-24T05:14:21Z

Codecov Report

❌ Patch coverage is 92.90323% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.04%. Comparing base (c4fc7a6) to head (7aeb9c3).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
sdk/src/builder.rs	88.73%	8 Missing ⚠️
sdk/src/reader.rs	56.25%	7 Missing ⚠️
sdk/src/assertions/bmff_hash.rs	33.33%	2 Missing ⚠️
sdk/src/claim.rs	90.47%	2 Missing ⚠️
sdk/src/identity/identity_assertion/assertion.rs	87.50%	2 Missing ⚠️
sdk/src/crypto/time_stamp/http_request.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1444      +/-   ##
==========================================
- Coverage   78.41%   78.04%   -0.38%     
==========================================
  Files         162      162              
  Lines       39515    40017     +502     
==========================================
+ Hits        30986    31230     +244     
- Misses       8529     8787     +258

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codspeed-hq · 2025-09-24T05:20:22Z

CodSpeed Performance Report

Merging #1444 will improve performances by 12.62%

_{Comparing scouten/settings-not-thread-local (7aeb9c3) with main (8bc2a1d)¹}

Summary

⚡ 16 improvements
⏩ 2 skipped²

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
⚡	`read 100kb-signed.gif (with manifest)`	6.1 ms	4.6 ms	+32.56%
⚡	`read 100kb-signed.jpg (with manifest)`	13.5 ms	12 ms	+12.62%
⚡	`read 100kb-signed.mp3 (with manifest)`	6.5 ms	5 ms	+29.57%
⚡	`read 100kb-signed.mp4 (with manifest)`	6.4 ms	4.9 ms	+30.82%
⚡	`read 100kb-signed.png (with manifest)`	13.6 ms	12.1 ms	+12.76%
⚡	`read 100kb-signed.svg (with manifest)`	10.1 ms	8.6 ms	+17.51%
⚡	`read 100kb-signed.tiff (with manifest)`	4.3 ms	2.8 ms	+53.84%
⚡	`read 100kb-signed.wav (with manifest)`	6.2 ms	4.7 ms	+31.63%
⚡	`sign 100kb gif`	6.5 ms	3.8 ms	+69.48%
⚡	`sign 100kb jpeg`	7.3 ms	5.3 ms	+37.28%
⚡	`sign 100kb mp3`	7 ms	4.6 ms	+51.49%
⚡	`sign 100kb mp4`	9.9 ms	5.7 ms	+73.43%
⚡	`sign 100kb png`	7.8 ms	5.7 ms	+37.12%
⚡	`sign 100kb svg`	19.6 ms	17 ms	+14.78%
⚡	`sign 100kb tiff`	4.8 ms	2.1 ms	×2.3
⚡	`sign 100kb wav`	6.9 ms	4.9 ms	+42.83%

No successful run was found on main (4e9c3f2) during the generation of this report, so 8bc2a1d was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

…ons? 🤞🏻🤞🏻🤞🏻

…gs passed in

sdk/src/assertions/bmff_hash.rs

tmathern · 2025-10-01T03:51:52Z

sdk/src/asset_handlers/bmff_io.rs

                let mut validation_log = StatusTracker::default();

+                let settings = Settings::default();
+                // ^^ TO REVIEW: Is there a reason we'd need non-default settings here?


Is there actually a setting already that changes things in here? If so, it could be useful to investigate why that setting came to be - might answer the question as side-effect.

I don't think this will be a problem, see here:
#1444 (comment)

sdk/src/builder.rs

tmathern · 2025-10-01T03:58:05Z

sdk/src/builder.rs

+                    //          "builder.auto_opened_action.source_type",
+                    //      );
+                    //
+                    // ... which omits the ".actions" node after "builder"


My 2 cents: It would be good to keep the settings structured. The missing actions node feels like a typo to me?

I agree. What I don't know is how to repair the test in question. @gpeacock?

The source type is only necessary for the c2pa.created action. We should be able to safely remove this from the test_settings.toml that the test uses.

@gpeacock for confirmation

scouten-adobe · 2025-10-01T16:42:59Z

sdk/tests/test_builder.rs

 use common::test_signer;

 #[test]
+#[ignore = "See discussion in builder.rs near line 1190"]


@gpeacock please review this before we merge.

#1444 (comment)

ok-nick

Leaving lots of reviews to track progress.

sdk/src/reader.rs

ok-nick · 2025-10-02T19:39:21Z

sdk/src/reader.rs

+        // TO DO BEFORE MERGE: Passing `verify` may be redundant now that we're
+        // passing settings.


TODO: internal

#1444 (comment)

ok-nick · 2025-10-02T19:39:26Z

sdk/src/reader.rs

+        // TO DO BEFORE MERGE: Passing `verify` may be redundant now that we're
+        // passing settings.


TODO: internal

#1444 (comment)

sdk/src/reader.rs

ok-nick · 2025-10-02T20:09:25Z

sdk/src/crypto/time_stamp/http_request.rs

+    // TO REVIEW: I think in this case, we're doing structural validation of
+    // time stamps but not trust validation. If so, the below is correct. (I think.)


TODO: internal

Yes, following the code path we explicitly set CertificateTrustPolicy::passthrough which stops before trust checks occur. A bigger question is if we need settings here at all versus passing verify_trust through the passthrough field of CertificateTrustPolicy.

ok-nick · 2025-10-02T20:09:38Z

sdk/src/crypto/cose/sigtst.rs

+    let settings = crate::settings::get_settings().unwrap_or_default();
+    // TO DO BEFORE MERGE? Pass Settings in here?


TODO: internal

#1444 (comment)

ok-nick · 2025-10-02T20:11:52Z

sdk/src/asset_handlers/bmff_io.rs

+        let settings = Settings::default();
+        // ^^ TO REVIEW: Is there a reason we'd need non-default settings here?
+        // If so, that means we hae to rework CAIReader and CAIWriter traits
+        // to take a &Settings arg and that is a pretty huge change.


I think Settings in the Store constructors are only used for configuring trust. In this scenario we are only using the Store to reconstruct the split manifest stores in BMFF, the trust is validated with a higher-level Store. It does feel wrong to create default settings and pass it in as a parameter here, but I don't think it'll be a problem for the time being.

cc @mauricefisher64 to confirm.

ok-nick · 2025-10-02T20:16:43Z

sdk/src/asset_handlers/bmff_io.rs

                let mut validation_log = StatusTracker::default();

+                let settings = Settings::default();
+                // ^^ TO REVIEW: Is there a reason we'd need non-default settings here?


I don't think this will be a problem, see here:
#1444 (comment)

sdk/src/assertions/bmff_hash.rs

… to construct stores in BMFF without settings

… separate PR)

scouten-adobe added 25 commits September 23, 2025 16:00

Spike: Retrofit io_utils call sites

44d8faa

Spike: Adapt verify_cose

e509d31

Spike: Retrofit Store::handle_remote_manifest

2a39a23

Spike: Retrofit sign_claim

73aead9

Spike: Push farther up the Store API chain

296aa51

Spike: Push Settings up through Ingredient API chain

ffd8015

Spike: Push settings up through Reader API

de83d42

Spike: Push settings farther up Builder and Ingredient API chains

5e638e8

Spike: Push settings up through thumbnail utils

b5093a4

Switch check_ingredient_trust to passed settings

023fde2

Plumb settings through core OCSP logic

427c7e1

Plumb settings through Store's OCSP logic

d3a078f

One more reference in ingredient

eb40a9f

Plumb settings through add_auto_actions_assertions_settings

ae3aee6

Plumb settings through more of Builder

929a396

Remove last references to get_settings_value in Builder

899b0ab

Remove remaining references to get_settings_value in Store

fbe8976

Remove remaining references to get_settings_value in Reader

c4e255c

Remove remaining references to get_settings_value in Claim

3a4d500

Remove remaining references to get_settings_value from IdentityAssertion

68ba524

Remove remaining get_settings_value references from time_stamp code

fce4aa8

Remove remaining references to get_settings_value outside Settings mod

beecb93

Move Settings::get_value to private

81efbdc

Move Settings::reset to private

3da6b20

Merge branch 'main' into scouten/settings-not-thread-local

c3de5a0

scouten-adobe self-assigned this Sep 24, 2025

Clippy

4608f62

cargo fmt

8941f2c

scouten-adobe added 7 commits September 30, 2025 18:49

Retrofit verify_time_stamp

263e13d

Retrofit Store::get_store_validation_info

0268023

TO REVIEW: Can we avoid passing settings into the BmffIO implementati…

98222b5

…ons? 🤞🏻🤞🏻🤞🏻

Retrofit Ingredient::add_to_claim

45e165f

Retrofit parse_and_validate_sigtst

f3352bf

TO REVIEW: I don't think default_rfc3161_request needs to have Settin…

b0bf042

…gs passed in

Clippy

1653c4f

scouten-adobe marked this pull request as ready for review October 1, 2025 02:45

scouten-adobe added 2 commits September 30, 2025 19:46

Merge branch 'main' into scouten/settings-not-thread-local

350f946

Fix up some Wasm build errors

9be45f6

scouten-adobe added the check-release Add this label to any PR to invoke a larger suite of tests. label Oct 1, 2025

scouten-adobe requested review from gpeacock and ok-nick October 1, 2025 02:58

tmathern reviewed Oct 1, 2025

View reviewed changes

scouten-adobe commented Oct 1, 2025

View reviewed changes

scouten-adobe assigned ok-nick and unassigned scouten-adobe Oct 1, 2025

ok-nick mentioned this pull request Oct 1, 2025

feat: impl ReaderBuilder #1465

Draft

ok-nick requested changes Oct 2, 2025

View reviewed changes

ok-nick mentioned this pull request Oct 2, 2025

Tracking issue for settings #1471

Open

10 tasks

ok-nick added 10 commits October 3, 2025 10:26

fix: use validation state from reader constructor

10b2f6a

refactor: simplify Reader

9551bcf

fix: private Ingredient::maybe_add_thumbnail and clean up todos

b2c687f

fix: Store::add with_settings and Store::from_jumbf_with_settings…

9f2cb0c

… to construct stores in BMFF without settings

fix: stack overflow when constructing store

ff0b160

style: fix clippy lints

4d6fb4d

fix: remove ingredient new public API for now

07dedf9

style: remove todo to add settings to identity assertion (deferred to…

779c1fe

… separate PR)

fix: remove public API changes to builder for now

4e605d0

style: remove todos in claim

7aeb9c3

		// TO DO BEFORE MERGE: Passing `verify` may be redundant now that we're
		// passing settings.

		// TO REVIEW: I think in this case, we're doing structural validation of
		// time stamps but not trust validation. If so, the below is correct. (I think.)

		let settings = crate::settings::get_settings().unwrap_or_default();
		// TO DO BEFORE MERGE? Pass Settings in here?

feat: Make settings not thread-local #1444

Are you sure you want to change the base?

feat: Make settings not thread-local #1444

Conversation

scouten-adobe commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codspeed-hq bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #1444 will improve performances by 12.62%

Summary

Benchmarks breakdown

Footnotes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ok-nick Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ok-nick Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ok-nick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ok-nick Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

scouten-adobe commented Sep 24, 2025 •

edited

Loading

codecov bot commented Sep 24, 2025 •

edited

Loading

codspeed-hq bot commented Sep 24, 2025 •

edited

Loading

ok-nick Oct 2, 2025 •

edited

Loading

ok-nick Oct 2, 2025 •

edited

Loading

ok-nick Oct 2, 2025 •

edited

Loading