Skip to content

fix: add TLS session resumption#1304

Open
mavonx wants to merge 6 commits into
floatpane:masterfrom
mavonx:fix/issue-1174
Open

fix: add TLS session resumption#1304
mavonx wants to merge 6 commits into
floatpane:masterfrom
mavonx:fix/issue-1174

Conversation

@mavonx
Copy link
Copy Markdown
Contributor

@mavonx mavonx commented May 17, 2026

What?

Configured TLS session resumption for IMAP and SMTP.

Why?

Reduce reconnect latency by avoiding full TLS handshakes.

Closes #1174

@mavonx mavonx requested a review from a team as a code owner May 17, 2026 22:15
@floatpanebot floatpanebot added bug Something isn't working area/fetcher IMAP fetch / IDLE / search area/sender SMTP send path area/config Configuration / settings size/S Diff: 11–50 lines and removed bug Something isn't working labels May 17, 2026
@andrinoff andrinoff added the bug Something isn't working label May 18, 2026
@andrinoff andrinoff requested a review from FromSi May 18, 2026 09:04
Comment thread fetcher/fetcher.go Outdated
Comment thread config/config.go Outdated
if a.ClientSessionCache == nil {
a.ClientSessionCache = tls.NewLRUClientSessionCache(64)
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the same account is used concurrently, for example by a background IMAP reconnect and an SMTP send, this can cause a data race during the first cache initialization. It would be better to initialize the cache up front when loading/creating the account, or protect it with a mutex / sync.Once.

@FromSi
Copy link
Copy Markdown
Member

FromSi commented May 18, 2026

@andrinoff Need to verify the expected behavior:

Verify session resumption is happening with a debug log.

I'm not entirely sure if this is implemented correctly or not.

@mavonx mavonx requested a review from FromSi May 18, 2026 18:46
@floatpanebot floatpanebot added the size/M Diff: 51–200 lines label May 18, 2026
@mavonx
Copy link
Copy Markdown
Contributor Author

mavonx commented May 18, 2026

@andrinoff Need to verify the expected behavior:

Verify session resumption is happening with a debug log.

I'm not entirely sure if this is implemented correctly or not.

@andrinoff @FromSi

I've verified the behavior in both scenarios:

  • Scenario 1
    • Log in with your account (no config.json yet).
    • Click on View Inbox. A new IMAP connection is established because there is no folder_cache.json. At this stage, I got cs.DidResume = false.
    • I fetched a new email body that was not present in the email_bodies folder, so a new IMAP connection was established again, but this time with cs.DidResume = true.
  • Scenario 2
    • Same as the first scenario, but this time we test with an already existing email, so the app reads directly from config.json.

@andrinoff
Copy link
Copy Markdown
Member

i will check it out tomorrow evening, im busy

@mavonx
Copy link
Copy Markdown
Contributor Author

mavonx commented May 19, 2026

I tried to handle the race condition as @FromSi suggested above, but tests failed with:

github.com/floatpane/matcha/config.Account contains sync.Once contains sync.noCopy

I moved to a pointer-based approach by extracting sync.Once and tls.ClientSessionCache into a separate SessionCache struct, but this introduced a new failure in TestSaveAndLoadConfigexpectedConfig creates accounts without SC, while LoadConfig now initializes them with SC: &SessionCache{} so reflect.DeepEqual sees a mismatch between nil and a non-nil pointer.

As a temporary fix I added SC: &SessionCache{} to the accounts in expectedConfig to match the loaded config, and the test passes. But I'd prefer not to touch the test.

Is there a cleaner approach here?

@floatpanebot floatpanebot added area/calendar Calendar integration size/L Diff: 201–800 lines labels May 20, 2026
Comment thread fetcher/fetcher.go
Comment on lines +379 to +383
ClientSessionCache: account.GetClientSessionCache(),
VerifyConnection: func(cs tls.ConnectionState) error {
log.Printf("IMAP TLS connection resumed: %t", cs.DidResume)
return nil
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will fire on every connection

Comment thread sender/sender.go
Comment on lines +664 to +668
ClientSessionCache: account.GetClientSessionCache(),
VerifyConnection: func(cs tls.ConnectionState) error {
log.Printf("SMTP TLS connection resumed: %t", cs.DidResume)
return nil
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will too

Comment thread sender/sender.go
Comment on lines +885 to +888
VerifyConnection: func(cs tls.ConnectionState) error {
log.Printf("SMTP TLS connection resumed: %t", cs.DidResume)
return nil
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

Comment thread config/config.go
@andrinoff
Copy link
Copy Markdown
Member

andrinoff commented May 20, 2026

@mavonx fix the debug logging.

image

this should not be the experience

@andrinoff
Copy link
Copy Markdown
Member

debug logging could be done by having an env variable (set with ENV_VAR=1 matcha) and it will then spit out.

@andrinoff andrinoff removed the size/L Diff: 201–800 lines label May 20, 2026
@andrinoff
Copy link
Copy Markdown
Member

actually, use #1311, just merged, and rebased the branch

@mavonx
Copy link
Copy Markdown
Contributor Author

mavonx commented May 20, 2026

GetClientSessionCache() panics if SC is nil. Won't happen right now, but better add a nil guard. better be safe, than sorry

@andrinoff

Regarding the nil guard — adding a nil check in GetClientSessionCache() would introduce a race condition: two goroutines could both see SC == nil simultaneously and each allocate a new &SessionCache{}, with one overwriting the other and losing cached sessions, so instead I initialize SC eagerly at every account creation site (new login, LoadConfig, and legacy migration) to eliminate the race condition — which is what forced TestSaveAndLoadConfig to need SC: &SessionCache{} in expectedConfig to match what LoadConfig returns.

Is that acceptable, or do you have a preferred approach to avoid touching the test?

@andrinoff
Copy link
Copy Markdown
Member

ok, that's fine then. do the log, i'll resolve the comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/calendar Calendar integration area/config Configuration / settings area/fetcher IMAP fetch / IDLE / search area/sender SMTP send path bug Something isn't working size/M Diff: 51–200 lines size/S Diff: 11–50 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: TLS sessions are not resumed across reconnects

4 participants