Refactor manager.go for improved safety and maintainability #1082

Muaz-Ibn-Kamal · 2025-09-16T19:04:12Z

This pull request refactors manager.go to improve safety, efficiency, and maintainability.

Changes made:

Added context parameter to NewSession and EndSession instead of using context.Background
Improved session cleanup by deleting session mapping before calling endSession
Preallocated slice in AvailableBrokers for better performance
Standardized and clarified error messages across methods
Normalized username handling in BrokerForUser

These changes make session handling safer, error messages clearer, and overall code more robust.

adombeck

Thanks for contributing!

In general, please don't bundle multiple changes into a single commit, but make commits as small as possible and explain the change in the commit message.

adombeck · 2025-09-17T08:54:57Z

internal/brokers/manager.go

 	}()

 	// Connect to the system bus
-	// Don't call dbus.SystemBus which caches globally system dbus (issues in tests)


Don't remove this comment

okey thank You

adombeck · 2025-09-17T09:09:38Z

internal/brokers/manager.go


 // AvailableBrokers returns currently loaded and available brokers in preference order.
 func (m *Manager) AvailableBrokers() (r []*Broker) {
+	r = make([]*Broker, 0, len(m.brokersOrder))


This change is fine, I see that it can avoid unnecessary allocations when appending the brokers. However, that's not explained in the commit message. Please create a separate commit with this change and explain in the commit message why it is useful.

adombeck · 2025-09-17T09:50:50Z

internal/brokers/manager.go

 	m.usersToBrokerMu.RLock()
 	defer m.usersToBrokerMu.RUnlock()
-	return m.usersToBroker[username]
+	return m.usersToBroker[strings.ToLower(username)]


I'm a bit undecided if we want this change or not. We do use lowercase usernames in authd, so this username must indeed be lowercase. However, we have so many layers of functions which take a username as an argument that it's a bit inefficient to convert it lowercase in each function.

Here we have these layers:

sequenceDiagram box PAM participant A as authd-pam end box authd participant B as pam.Service participant C as brokers.Manager end Note over A,B: Communication via gRPC A->>B: GetPreviousBroker B->>C: BrokerForUser C-->>B: Return broker B-->>A: Response

Loading

If the brokers.Manager doesn't find the broker, the users.Manager is called, which results in even more layers:

sequenceDiagram box PAM participant A as authd-pam end box authd participant B as pam.Service participant C as users.Manager participant D as db.Manager end Note over A,B: Communication via gRPC A->>B: GetPreviousBroker B->>C: BrokerForUser C->>D: UserByName D-->>C: UserRow.BrokerID C-->>B: Return BrokerID B-->>A: Response

Loading

We currently call strings.ToLower in all of these layers (or at least in some functions of each layer).

Looking at it like this, it's my impression that it would make most sense to just convert the usernames to lowercase once in the gRPC methods, and in the other layers (brokers.Manager, users.Manager, db.Manager) assume that the usernames are already in lowercase.

Note that the same applies to all other methods of pam.Service which takes a username as an argument, and also to the methods of user.Service.

What do you think, @3v1n0?

Yeah, I was noticing the change too and I was wondering whether is needed... Ideally we should do it in any public APIs though at this point...

adombeck · 2025-09-17T09:53:27Z

internal/brokers/manager.go

 	broker, exists := m.transactionsToBroker[id]
 	if !exists {
-		return nil, fmt.Errorf("no broker found for session %q", id)
+		return nil, fmt.Errorf("no broker found for session ID %q", id)


We use "session %q" all over the place in error and debug messages, so for consistency we should not change it here.

adombeck · 2025-09-17T09:54:10Z

internal/brokers/manager.go


-// NewSession create a new session for the broker and store the sesssionID on the manager.
-func (m *Manager) NewSession(brokerID, username, lang, mode string) (sessionID string, encryptionKey string, err error) {
+// NewSession create a new session for the broker and store the sessionID on the manager.


Please create a separate commit with this change. The commit message can simply be "Fix typo".

I see you pushed a commit with the commit message "Fix typo". However, it doesn't contain this change but adds a comment //Fix typo.

adombeck · 2025-09-17T09:55:46Z

internal/brokers/manager.go

-// NewSession create a new session for the broker and store the sesssionID on the manager.
-func (m *Manager) NewSession(brokerID, username, lang, mode string) (sessionID string, encryptionKey string, err error) {
+// NewSession create a new session for the broker and store the sessionID on the manager.
+func (m *Manager) NewSession(ctx context.Context, brokerID, username, lang, mode string) (sessionID string, encryptionKey string, err error) {


Let's not add unused context parameters to functions. The log functions don't actually use their context arguments (for now), so that's not a good reason to add a context parameter here.

adombeck · 2025-09-17T09:58:18Z

internal/brokers/manager.go

 // EndSession signals the end of the session to the broker associated with the sessionID and then removes the
 // session -> broker mapping.
-func (m *Manager) EndSession(sessionID string) error {
-	b, err := m.BrokerFromSessionID(sessionID)
-	if err != nil {
-		return err
+func (m *Manager) EndSession(ctx context.Context, sessionID string) error {
+	m.transactionsToBrokerMu.Lock()
+	broker, exists := m.transactionsToBroker[sessionID]
+	if !exists {
+		m.transactionsToBrokerMu.Unlock()
+		return fmt.Errorf("no broker found for session ID %q", sessionID)
 	}


why should we change the order of deleting session mapping and calling endSession? if we want to change this (which I'm not convinced of), we should also update the comment.

adombeck · 2025-09-17T09:59:28Z

internal/brokers/manager.go

 	broker, exists := m.brokers[id]
 	if !exists {
-		return nil, fmt.Errorf("no broker found matching %q", id)
+		return nil, fmt.Errorf("no broker found matching ID %q", id)


I'm fine with this change, please create a separate commit for it.

internal/brokers/manager.go

Muaz-Ibn-Kamal · 2025-09-18T15:52:01Z

internal/brokers/manager.go


 // AvailableBrokers returns currently loaded and available brokers in preference order.
 func (m *Manager) AvailableBrokers() (r []*Broker) {
+	r = make([]*Broker, 0, len(m.brokersOrder))


Optimize AvailableBrokers to preallocate slice capacity

Preallocate the slice in AvailableBrokers with len(m.brokersOrder) capacity:

r = make([]*Broker, 0, len(m.brokersOrder))

This avoids repeated slice reallocations when appending brokers,
improves efficiency, and reduces memory churn, especially when
many brokers are loaded.

True, we don't have that many though... But well still fine.

Optimize AvailableBrokers to preallocate slice capacity

Preallocate the slice in AvailableBrokers with len(m.brokersOrder) capacity:

r = make([]*Broker, 0, len(m.brokersOrder))

This avoids repeated slice reallocations when appending brokers,
improves efficiency, and reduces memory churn, especially when
many brokers are loaded.

This would be a good commit message. Again, please create a separate commit with this change.

Refactor manager.go for improved safety and maintainability

890ec94

Muaz-Ibn-Kamal requested a review from a team as a code owner September 16, 2025 19:04

adombeck requested changes Sep 17, 2025

View reviewed changes

adombeck mentioned this pull request Sep 17, 2025

Refactor manager.go for improved safety and maintainability #1081

Closed

Merge branch 'main' into patch-1

0158059

Muaz-Ibn-Kamal commented Sep 18, 2025

View reviewed changes

Muaz-Ibn-Kamal added 2 commits September 18, 2025 22:13

Fix error message to include ID in broker lookup

35661ed

Fix typo

fe7ca67

Refactor manager.go for improved safety and maintainability #1082

Are you sure you want to change the base?

Refactor manager.go for improved safety and maintainability #1082

Uh oh!

Conversation

Muaz-Ibn-Kamal commented Sep 16, 2025

Uh oh!

adombeck left a comment

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!

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!

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!

Uh oh!