Make sync-experiment Tiled-based (rebased) by nmaytan · Pull Request #307 · NSLS2/nslsii

Nathanael (Nate) Maytan (nmaytan) · 2026-10-08T01:39:31Z

See #234 for previous notes/context. This rebased PR supersedes the old one. It includes some additional fixes and improvements.

Some work is still needed after the rebase:

The Redis client/db used for API key store needs to be made to use secure Redis, and also needs a dedicated db index (or maybe not depending how prefixing works in Redis now?)
We should make secure Redis default to True? Or even better, remove this flag completely and only use secure Redis?
Normalized beamline vs not for Redis location and prefix? Get on the same page about prefixing in general (and greedy RedisJSONDict ingestion)
The host resolver needs to be retooled to be more robust
~~Check that pixi has the right requirements for the new version~~
~~Add Redis parameters to doctstrings~~
~~Double check that I strung the Redis parameters through everywhere~~
~~Possibly reorder the argsparse section, as well as the added function arguments~~

And some things to callout:

I added the beamline TLA to the Redis location and prefix for better disambiguation - avoids endstation/endstation and endstation/beamline naming conflicts (multiple beamlines may have an ARPES endstation, for example)
- I had done this for the key prefixes in the previous PR, but the rebase is almost all Redis changes, so I point it out explicitly here
I changed the enable-ssl flag to capitalized -S, since -s is used in the new version for an important function (switching between authorized proposals)

I also updated the pixi-.lock after dependency changes:

$ pixi lock
[...]
✔ Updated lock file
Environment: default
  - P ldap3           2.9.1
  - P pyasn1          0.6.4

Environment: dev
  - P ldap3           2.9.1
  - P pyasn1          0.6.4
  + P pytest-asyncio  1.4.0

Environment: py311
  - P ldap3           2.9.1
  - P pyasn1          0.6.4
  + P pytest-asyncio  1.4.0

Environment: py312
  - P ldap3           2.9.1
  - P pyasn1          0.6.4
  + P pytest-asyncio  1.4.0

Environment: py313
  - P ldap3           2.9.1
  - P pyasn1          0.6.4
  + P pytest-asyncio  1.4.0

And hardened the decisions made about when to clear the active API key, or retain it and insist upon successful revocation:

Situation	HTTP / Result	Redis Key	Operation
No key stored	No request	Unchanged / empty	Continues
Valid key, revocation succeeds	`204`	Cleared	Continues
Key expired before context creation	`401`	Cleared	Continues
Key expired before `which_api_key()`	`401`	Cleared	Continues
Key manually deleted before validation	`401`	Cleared	Continues
Valid key lacks revoke scope	`401` from revoke	Retained	Aborts
Revocation explicitly forbidden	`403`	Retained	Aborts
Key disappears after validation	`404` from revoke	Cleared	Continues
Network error or timeout	Request error	Retained	Aborts
Tiled server error	`5xx`	Retained	Aborts
Unexpected context creation error	Any other error	Retained	Aborts
Redis clearing fails after revocation	Redis error	Possibly retained	Aborts; retry later detects unusable key
`logout()` or `close()` fails	Cleanup error	Already cleared or retained based on preceding result	Aborts

Experiments (proposals) are "authorized". Then, one of the authorized experiments is "activated". Multiple proposals may be authorized at the same time, but only one proposal can be active at a time. This should make more clear what the tool is doing, and how the policy works, as opposed to before (authorize/activate vs activate/select or activate/load). Includes some other minor word fixes.

…ormalized_beamline

Copilot

🟡 Changes recommended

CLI failures, inconsistent Redis handling, and insecure API-key transport and revocation block approval.

9 open findings

Declare the required Tiled dependency · New
Secure API-key Redis storage with TLS and authentication · New
Migrate Redis integration tests to switch_proposal · New
Resolve duplicate -s argument registration · New
Preserve failed revocations for retry · New
Use normalized beamline for composite Redis host resolution · New
Safely handle missing unsync metadata fields · New
Use store_true for the unsync operation flag · New
Support Redis database and TLS settings in unsync · New

What changed in this PR

Migrates beamline experiment authentication from LDAP to Tiled, continuing #234’s multi-proposal workflow.

Changes:

Adds proposal authorization, switching, and unsync operations.
Introduces Redis-backed API-key storage and revocation.
Updates beamline/endstation namespaces and CLI options.

File	Description
`nslsii/sync_experiment/sync_experiment.py`	Implements Tiled authentication, proposal workflows, and Redis integration.
`nslsii/sync_experiment/__init__.py`	Exports the new sync, unsync, and switching APIs.

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Nathanael (Nate) Maytan (nmaytan) · 2026-10-08T01:52:38Z

+    apikey_redis_client = redis.Redis(
+        host=f"info.{normalized_beamline}.nsls2.bnl.gov",
+        port=6379,
+        db=15,
+        decode_responses=True,


This is known and mentioned in the OP already.

Nathanael (Nate) Maytan (nmaytan) · 2026-10-08T02:17:19Z

-    return proposal_data
+    md_redis_client = open_redis_client(
+        redis_ssl=redis_ssl,
+        redis_location=redis_location,


This is one of the things I needed to dig into from the prefix change. However, the hostname list in the utils should be removed, and the resolver needs to be made more robust than its current naive sub-string check.

Copilot

🟡 Changes recommended

Unresolved synchronization, Redis routing, and metadata compatibility issues can leave experiment state inconsistent or prevent operations.

8 open findings

Race condition can mix active keys and proposal metadata · New
Align metadata prefix with RunEngine Redis configuration · New
Migrate Redis integration tests to switch_proposal
Secure API-key Redis storage with TLS and authentication
Validate activate ID zero and empty proposal lists · New
Use separate Redis host selection in unsync · New
Handle context and logout failures during sync and unsync · New
Use normalized beamline for composite Redis host resolution

6 resolved since last review

Resolve duplicate -s argument registration
Declare the required Tiled dependency
Support Redis database and TLS settings in unsync
Use store_true for the unsync operation flag
Safely handle missing unsync metadata fields
Preserve failed revocations for retry

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot

🟡 Changes recommended

Redis normalization causes Tiled profile selection to reject valid SST beamline profiles.

3 open findings

Secure API-key Redis storage with TLS and authentication
Use normalized beamline for composite Redis host resolution
Add missing revocation and cleanup regression tests · New

6 resolved since last review

Align metadata prefix with RunEngine Redis configuration
Race condition can mix active keys and proposal metadata
Migrate Redis integration tests to switch_proposal
Handle context and logout failures during sync and unsync
Use separate Redis host selection in unsync
Validate activate ID zero and empty proposal lists

Previously missed (2)

In code that hasn't changed since last review

Preserve beamline for Tiled profile lookup

nslsii/sync_experiment/sync_experiment.py:115

For beamline='sst1' with an sst1 Tiled profile and no endstation, this passes sst to profile lookup and raises Cannot find Tiled profile, ignoring the configured profile. Revocation uses the same normalized name. Preserve the requested beamline for Tiled profile selection in both authentication and revocation, and pass the Redis namespace separately.

Restore successful-response error payload handling

nslsii/sync_experiment/sync_experiment.py:659

The removed validate_proposal handled an error_message inside the proposal payload even when HTTP status was successful. That response now reaches proposal['instruments'] and raises KeyError, hiding the server's explanation. Restore the error-payload check before accessing proposal fields.

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

+    tiled_context.revoke_api_key(first_eight)
+
+
+def revoke_active_api_key(redis_client, beamline, endstation):


Nathanael (Nate) Maytan (nmaytan) added 26 commits October 7, 2026 21:05

Draft of Tiled-based sync-experiment

6510445

Cleanups

ddd82b1

Smoothing out wrinkles

18dd94b

Add catalog root node tags to API keys

497f5b5

Upper the beamline in the root node tag

88b811e

Treat decrypt failure as missing key

e7816a7

Fix vars in cache PQ

1def64e

Add note about duo prompt to login message

da5d598

Add unsync_experiment, update switch_proposal, correct use cases of n…

b74e83c

…ormalized_beamline

Sort data_sessions used for Redis keys to ensure determinism

2f03a2f

Make switch_proposal work from CLI

c65d6a9

Fix method name for authz check

7c8b8dc

Remove redundant lowers for beamline

e8488b2

Add 'ts' for timestamp keys

cb03a26

Use __missing__ to default normalized beamline

4037a73

Add prompt for Duo passcode or method

6846a79

Minor exception tweaks

a063a28

Update docstrings

f479cf1

Fix access_tags dtype, update prints

2e253f1

Restrict scopes on generated apikeys

03deff5

Remove apikeys scope

10d0005

Revoke api keys instead of caching

cd23069

Change from root_node tag to public tag

9195e56

Make sync-experiment work with Tiled Entra auth

ee85a39

Bug fix from switch to OIDC

0e432ad

Copilot AI balanced review requested due to automatic review settings October 8, 2026 01:39

Copilot started reviewing on behalf of Nathanael (Nate) Maytan (nmaytan) October 8, 2026 01:40 View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

Cleanups and docstrings for redis params

887037c

Nathanael (Nate) Maytan (nmaytan) added 3 commits October 7, 2026 22:45

Update dependencies and pixi.lock

d4891fb

Reorder argsparse args

286f530

Minor improvements

2e6e841

Copilot AI balanced review requested due to automatic review settings October 8, 2026 03:01

Copilot started reviewing on behalf of Nathanael (Nate) Maytan (nmaytan) October 8, 2026 03:02 View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

Nathanael (Nate) Maytan (nmaytan) added 2 commits October 7, 2026 23:14

Fix switch_proposal tests (done by Sol-5.6)

ae9b895

Harden decision around API key clearing, better client cleanup

bbe2889

Copilot AI balanced review requested due to automatic review settings October 8, 2026 04:15

Copilot started reviewing on behalf of Nathanael (Nate) Maytan (nmaytan) October 8, 2026 04:16 View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

Comment thread nslsii/sync_experiment/sync_experiment.py

tiled_context.revoke_api_key(first_eight)

def revoke_active_api_key(redis_client, beamline, endstation):

Navigation Menu

Repository navigation

Make sync-experiment Tiled-based (rebased) - #307

Make sync-experiment Tiled-based (rebased)#307
Nathanael (Nate) Maytan (nmaytan) wants to merge 32 commits into
NSLS2:mainfrom
nmaytan:sync_experiment_tiled-on-main

Nathanael (Nate) Maytan (nmaytan) commented Oct 8, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Nathanael (Nate) Maytan (nmaytan) Oct 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nathanael (Nate) Maytan (nmaytan) Oct 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		tiled_context.revoke_api_key(first_eight)


		def revoke_active_api_key(redis_client, beamline, endstation):

Conversation

Nathanael (Nate) Maytan (nmaytan) commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Nathanael (Nate) Maytan (nmaytan) Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nathanael (Nate) Maytan (nmaytan) Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Nathanael (Nate) Maytan (nmaytan) commented Oct 8, 2026 •

edited

Loading