feat: logout, users/me, user count, session timeout #24
No reviewers
Labels
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/chat#24
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "feature/mvp-remaining"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Remaining MVP endpoints:
POST /api/v1/logout— deletes the authenticated client tokenGET /api/v1/users/me— delegates to existing /state handlerGET /api/v1/server— now includesuserscountSESSION_IDLE_TIMEOUTenv (default 24h), periodic cleanup goroutine removes stale clients and empty sessionsmake check: all tests pass, 0 lint issues, build succeeds.Note: This branch is based on current main. If PR #23 (auth-passwords) merges first, this will need a rebase.
- POST /api/v1/logout: deletes client token, returns {status: ok} - GET /api/v1/users/me: returns session info (delegates to HandleState) - Add DeleteClient, GetSessionCount, ClientCountForSession, DeleteStaleSessions to db layer - Add user count to GET /api/v1/server response - Extract setupAPIv1 to fix funlen lint issueCode Review
LGTM
✅ No linter/config/Makefile changes
✅ Logout properly deletes client record via
DeleteClient()✅ UsersMe delegates to existing HandleState (no duplication)
✅ Server info now includes session count via
GetSessionCount()✅ Session timeout: configurable via env, cleanup goroutine with proper shutdown via context cancellation
✅
make checkpasses: all tests green, 0 lint issues, build succeedsAssigning to sneak.
Reviewed and verified —
make checkpasses clean (all tests green, 0 lint issues, build OK).Looks good: logout deletes client token,
/users/medelegates to state, server info now includes user count, idle cleanup goroutine with configurable timeout.Note: PR description mentions this is based on main and will need a rebase if #23 merges first.
Assigned to sneak for merge.
Pipeline Review
✅ Rebase on main: clean
✅
make check: all tests pass, 0 lint issues, build OK✅ Code review: LGTM
Summary: Adds logout (
POST /api/v1/logout— deletes client token),/api/v1/users/me(alias for state), user count in/api/v1/serverresponse, configurableSESSION_IDLE_TIMEOUTwith background cleanup goroutine that removes stale clients and orphaned sessions. Clean implementation with proper context cancellation on shutdown.max()builtin used correctly for interval calculation.Labeled
merge-ready, assigned to @sneak.PR 23 has indeed been merged first.
2774be06d4toe7dca3a021e7dca3a021tobdc243224bfeat: logout, users/me, user count, session timeout (closes #1)to feat: logout, users/me, user count, session timeoutRebased onto main (after PR #23 merge). Resolved merge conflict in
internal/server/routes.go— kept both register/login routes from #23 and logout/users-me routes from this PR.docker build .passes. Spawning review agent.Rebased
feature/mvp-remainingonto currentmain(post-PR#23 merge). Resolved merge conflict ininternal/server/routes.go— kept both the register/login routes from #23 and the logout/users-me routes from this branch.docker build .passes (all tests green, lint clean, 71.6% coverage). Force-pushed.Code Review — Issues Found
Bug: Cleanup goroutine uses OnStart context (will stop prematurely)
In
handlers.go,startCleanupis called from thefx.OnStarthook and receivesctxfrom that hook.startCleanupthen derives a child context viacontext.WithCancel(ctx).In
fx, theOnStartcontext is startup-scoped — it has a timeout and is cancelled once all start hooks complete. This meanscleanupCtxwill be cancelled shortly after startup, and the cleanup goroutine will exit on<-ctx.Done().Fix: Use
context.Background()instead of the OnStartctxas the parent for the cleanup context. ThecancelCleanupfunction called fromOnStopprovides the lifecycle control.Minor observations
GetSessionCountcounts sessions, but the API field isusers. These may diverge if a user can have multiple sessions. Consider naming itsessionsoractive_sessionsfor accuracy.HandleLogoutdoes not immediately clean up orphaned sessions (the cleanup loop handles it eventually). This is fine but worth documenting.The context bug is a functional defect — the idle timeout cleanup will silently stop working after app startup.
the OnStart ctx must be fixed.
a session can have multiple clients, but the session-nick (user) mapping is 1-1. call it 'user' everywhere, not 'session'. make logout clean everything up properly.
@clawbot ping
All three items from sneak's review are addressed in commit
910a5c2:startCleanupnow usescontext.Background()instead of the startup-scoped OnStart ctxGetSessionCount→GetUserCount,DeleteStaleSessions→DeleteStaleUsers,cleanupSession→cleanupUserHandleLogoutnow fully cleans up when last client disconnects: parts all channels (sends QUIT to members), removes empty channels, deletes the session recorddocker build .passes. Spawning review agent.Code Review (post-rework)
✅
docker build .passes.All three items from sneak's feedback are addressed:
startCleanupnow correctly usescontext.Background()instead of the startup-scoped OnStart ctx. The//nolint:contextcheckannotation is appropriate.GetUserCount,DeleteStaleUsers,cleanupUser— consistent "user" terminology throughout.HandleLogoutchecks remaining client count; when last client disconnects,cleanupUserparts all channels (sends QUIT to members), deletes empty channels, and removes the session.Minor observation (non-blocking): The background idle cleanup (
DeleteStaleUsers) removes stale clients/sessions directly via SQL without sending QUIT notifications to channel members. This means timed-out users silently disappear from channels. The explicit logout path handles this correctly. This is acceptable for idle timeout behavior but worth noting for future consideration.Labeling
merge-ready, assigning to @sneak.@clawbot please fix the identified background idle cleanup issue.
Rework: Background idle cleanup now sends QUIT notifications
Fixed the issue where
runCleanupwas callingDeleteStaleUsersdirectly, which removed stale clients/sessions via SQL without notifying channel members.Changes
internal/db/queries.go— AddedGetStaleOrphanSessions()method that identifies sessions where all clients have alast_seenbefore the cutoff. These are the sessions that will become orphaned when stale clients are deleted.internal/handlers/handlers.go— ModifiedrunCleanup()to callGetStaleOrphanSessions()first, thencleanupUser()for each stale session (sending QUIT notifications and parting channels), before callingDeleteStaleUsers()to do the actual deletion.internal/handlers/api.go— RefactoredcleanupUser()to acceptcontext.Contextinstead of*http.Request, so it can be called from both the HTTP logout handler and the background cleanup goroutine.Result
Timed-out users now get proper QUIT notifications sent to all channel members, matching the explicit logout behavior.
docker build .passes (lint, tests, build).Code Review — FAIL (README inconsistencies)
Code: ✅ Correct
The implementation is solid:
GetStaleOrphanSessions()correctly identifies sessions where ALL clients are stale (IN stale, NOT IN non-stale)runCleanupproperly callscleanupUserfor each stale session beforeDeleteStaleUserscleanupUserworks correctly withcontext.Context— no regressions in HTTP handler path (HandleLogoutpassesrequest.Context())notifiedmap)clients.session_idmeansDeleteSessionincleanupUsercascades to clients; subsequentDeleteStaleUsersharmlessly finds nothing — safeMakefilechangesdocker build .passesREADME: ❌ Not updated
The README must be updated to reflect the new functionality:
GET /api/v1/serverresponse — README line ~1168 shows{"name": ..., "motd": ...}but the code now returns{"name": ..., "motd": ..., "users": ...}. Add theusersfield.POST /api/v1/logoutendpoint — Not documented anywhere in the README. Needs its own section in the API Reference.GET /api/v1/users/meendpoint — Not documented. Needs its own section (can note it returns the same response asGET /api/v1/state).Configuration table — README line ~1593 documents
SESSION_TIMEOUT(int, seconds, "planned") but the actual implementation usesSESSION_IDLE_TIMEOUT(string, Go duration like24h). Either:SESSION_TIMEOUTwithSESSION_IDLE_TIMEOUTand update type/description, orSessionTimeoutfield from config and useSESSION_IDLE_TIMEOUTconsistentlyStorage section — README line ~1575 says session expiry is "planned" — it is now implemented.
Dead config field —
SessionTimeout intinconfig.gois loaded but never read by any code. The actual timeout usesSessionIdleTimeout string. Clean up the dead field or unify them.Rework complete. All 6 review findings addressed:
GET /api/v1/serverresponse — addedusersfield to documented response with field tablePOST /api/v1/logout— added full API Reference section with request/response/errors/curl exampleGET /api/v1/users/me— added full API Reference section (documents it as alias forGET /api/v1/state)SESSION_TIMEOUT→SESSION_IDLE_TIMEOUT(type: string, default:24h), also updated.envexampleSessionTimeoutconfig field — removed field, viper default, and viper read frominternal/config/config.goAlso updated Roadmap: moved session expiry, logout, users/me, and user count to Implemented section.
make fmt✅ |docker build .✅ | All tests pass ✅Re-review: PASS ✅
All 6 findings from the previous review are fixed:
usersfield documented inGET /api/v1/serverresponsePOST /api/v1/logoutendpoint fully documentedGET /api/v1/users/meendpoint fully documentedSESSION_IDLE_TIMEOUT(notSESSION_TIMEOUT)SessionTimeoutconfig field removed from Go codedocker build .passes. No regressions from the dead config removal. PR is up to date with main.