POST /api/v1/logout — deletes the authenticated client token
GET /api/v1/users/me — delegates to existing /state handler
GET /api/v1/server — now includes users count
Session idle timeout — configurable via SESSION_IDLE_TIMEOUT env (default 24h), periodic cleanup goroutine removes stale clients and empty sessions
make 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.
Remaining MVP endpoints:
1. **`POST /api/v1/logout`** — deletes the authenticated client token
2. **`GET /api/v1/users/me`** — delegates to existing /state handler
3. **`GET /api/v1/server`** — now includes `users` count
4. **Session idle timeout** — configurable via `SESSION_IDLE_TIMEOUT` env (default 24h), periodic cleanup goroutine removes stale clients and empty sessions
**`make 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.
clawbot
self-assigned this 2026-02-27 14:07:57 +01:00
- 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 issue
- Periodic cleanup loop deletes stale clients based on SESSION_IDLE_TIMEOUT
- Orphaned sessions (no clients) are cleaned up automatically
- last_seen already updated on each authenticated request via GetSessionByToken
✅ 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 check passes: all tests green, 0 lint issues, build succeeds
Assigning to sneak.
## Code 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 check` passes: all tests green, 0 lint issues, build succeeds
Assigning to sneak.
clawbot
removed their assignment 2026-02-27 14:08:08 +01:00
sneak
was assigned by clawbot2026-02-27 14:08:08 +01:00
Reviewed and verified — make check passes clean (all tests green, 0 lint issues, build OK).
Looks good: logout deletes client token, /users/me delegates 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.
Reviewed and verified — `make check` passes clean (all tests green, 0 lint issues, build OK).
Looks good: logout deletes client token, `/users/me` delegates 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.
✅ 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/server response, configurable SESSION_IDLE_TIMEOUT with 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.
## 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/server` response, configurable `SESSION_IDLE_TIMEOUT` with 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.
clawbot
changed title from feat: logout, users/me, user count, session timeout (closes #1) to feat: logout, users/me, user count, session timeout2026-02-28 20:00:51 +01:00
Rebased 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 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-remaining onto current main (post-PR#23 merge). Resolved merge conflict in internal/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.
Rebased `feature/mvp-remaining` onto current `main` (post-PR#23 merge). Resolved merge conflict in `internal/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.
sneak
was assigned by clawbot2026-02-28 20:00:58 +01:00
In handlers.go, startCleanup is called from the fx.OnStart hook and receives ctx from that hook. startCleanup then derives a child context via context.WithCancel(ctx).
In fx, the OnStart context is startup-scoped — it has a timeout and is cancelled once all start hooks complete. This means cleanupCtx will be cancelled shortly after startup, and the cleanup goroutine will exit on <-ctx.Done().
Fix: Use context.Background() instead of the OnStart ctx as the parent for the cleanup context. The cancelCleanup function called from OnStop provides the lifecycle control.
Minor observations
GetSessionCount counts sessions, but the API field is users. These may diverge if a user can have multiple sessions. Consider naming it sessions or active_sessions for accuracy.
HandleLogout does 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.
## Code Review — Issues Found
### Bug: Cleanup goroutine uses OnStart context (will stop prematurely)
In `handlers.go`, `startCleanup` is called from the `fx.OnStart` hook and receives `ctx` from that hook. `startCleanup` then derives a child context via `context.WithCancel(ctx)`.
In `fx`, the `OnStart` context is **startup-scoped** — it has a timeout and is cancelled once all start hooks complete. This means `cleanupCtx` will be cancelled shortly after startup, and the cleanup goroutine will exit on `<-ctx.Done()`.
**Fix:** Use `context.Background()` instead of the OnStart `ctx` as the parent for the cleanup context. The `cancelCleanup` function called from `OnStop` provides the lifecycle control.
### Minor observations
- `GetSessionCount` counts sessions, but the API field is `users`. These may diverge if a user can have multiple sessions. Consider naming it `sessions` or `active_sessions` for accuracy.
- `HandleLogout` does 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.
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.
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.
sneak
removed their assignment 2026-02-28 20:05:15 +01:00
- Use context.Background() for cleanup goroutine instead of
OnStart ctx which is cancelled after startup completes
- Rename GetSessionCount→GetUserCount, DeleteStaleSessions→
DeleteStaleUsers to reflect that sessions represent users
- HandleLogout now fully cleans up when last client disconnects:
parts all channels (notifying members via QUIT), removes
empty channels, and deletes the session/user record
- docker build passes, all tests green, 0 lint issues
Logout cleanup — HandleLogout now fully cleans up when last client disconnects: parts all channels (sends QUIT to members), removes empty channels, deletes the session record
docker build . passes. Spawning review agent.
All three items from sneak's review are addressed in commit 910a5c2:
1. **OnStart ctx fix** — `startCleanup` now uses `context.Background()` instead of the startup-scoped OnStart ctx
2. **Renamed session→user** — `GetSessionCount`→`GetUserCount`, `DeleteStaleSessions`→`DeleteStaleUsers`, `cleanupSession`→`cleanupUser`
3. **Logout cleanup** — `HandleLogout` now fully cleans up when last client disconnects: parts all channels (sends QUIT to members), removes empty channels, deletes the session record
`docker build .` passes. Spawning review agent.
All three items from sneak's feedback are addressed:
OnStart ctx fix — startCleanup now correctly uses context.Background() instead of the startup-scoped OnStart ctx. The //nolint:contextcheck annotation is appropriate.
Logout cleanup — HandleLogout checks remaining client count; when last client disconnects, cleanupUser parts 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.
## Code Review (post-rework)
✅ `docker build .` passes.
All three items from sneak's feedback are addressed:
1. **OnStart ctx fix** — `startCleanup` now correctly uses `context.Background()` instead of the startup-scoped OnStart ctx. The `//nolint:contextcheck` annotation is appropriate.
2. **Renamed session→user** — `GetUserCount`, `DeleteStaleUsers`, `cleanupUser` — consistent "user" terminology throughout.
3. **Logout cleanup** — `HandleLogout` checks remaining client count; when last client disconnects, `cleanupUser` parts 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.
The background idle cleanup (DeleteStaleUsers) was removing stale
clients/sessions directly via SQL without sending QUIT notifications
to channel members. This caused timed-out users to silently disappear
from channels.
Now runCleanup identifies sessions that will be orphaned by the stale
client deletion and calls cleanupUser for each one first, ensuring
QUIT messages are sent to all channel members — matching the explicit
logout behavior.
Also refactored cleanupUser to accept context.Context instead of
*http.Request so it can be called from both HTTP handlers and the
background cleanup goroutine.
Rework: Background idle cleanup now sends QUIT notifications
Fixed the issue where runCleanup was calling DeleteStaleUsers directly, which removed stale clients/sessions via SQL without notifying channel members.
Changes
internal/db/queries.go — Added GetStaleOrphanSessions() method that identifies sessions where all clients have a last_seen before the cutoff. These are the sessions that will become orphaned when stale clients are deleted.
internal/handlers/handlers.go — Modified runCleanup() to call GetStaleOrphanSessions() first, then cleanupUser() for each stale session (sending QUIT notifications and parting channels), before calling DeleteStaleUsers() to do the actual deletion.
internal/handlers/api.go — Refactored cleanupUser() to accept context.Context instead 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).
## Rework: Background idle cleanup now sends QUIT notifications
Fixed the issue where `runCleanup` was calling `DeleteStaleUsers` directly, which removed stale clients/sessions via SQL without notifying channel members.
### Changes
1. **`internal/db/queries.go`** — Added `GetStaleOrphanSessions()` method that identifies sessions where all clients have a `last_seen` before the cutoff. These are the sessions that will become orphaned when stale clients are deleted.
2. **`internal/handlers/handlers.go`** — Modified `runCleanup()` to call `GetStaleOrphanSessions()` first, then `cleanupUser()` for each stale session (sending QUIT notifications and parting channels), before calling `DeleteStaleUsers()` to do the actual deletion.
3. **`internal/handlers/api.go`** — Refactored `cleanupUser()` to accept `context.Context` instead 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).
GetStaleOrphanSessions() correctly identifies sessions where ALL clients are stale (IN stale, NOT IN non-stale)
runCleanup properly calls cleanupUser for each stale session before DeleteStaleUsers
cleanupUser works correctly with context.Context — no regressions in HTTP handler path (HandleLogout passes request.Context())
QUIT notifications properly sent to all channel members (deduplicated via notified map)
CASCADE on clients.session_id means DeleteSession in cleanupUser cascades to clients; subsequent DeleteStaleUsers harmlessly finds nothing — safe
No concurrency issues: SQLite serializes writes, cleanup goroutine and HTTP handlers share the DB safely
No cheating: no test changes, no linter config changes, no Makefile changes
docker build . passes
README: ❌ Not updated
The README must be updated to reflect the new functionality:
GET /api/v1/server response — README line ~1168 shows {"name": ..., "motd": ...} but the code now returns {"name": ..., "motd": ..., "users": ...}. Add the users field.
POST /api/v1/logout endpoint — Not documented anywhere in the README. Needs its own section in the API Reference.
GET /api/v1/users/me endpoint — Not documented. Needs its own section (can note it returns the same response as GET /api/v1/state).
Configuration table — README line ~1593 documents SESSION_TIMEOUT (int, seconds, "planned") but the actual implementation uses SESSION_IDLE_TIMEOUT (string, Go duration like 24h). Either:
Replace SESSION_TIMEOUT with SESSION_IDLE_TIMEOUT and update type/description, or
Remove the dead SessionTimeout field from config and use SESSION_IDLE_TIMEOUT consistently
Storage section — README line ~1575 says session expiry is "planned" — it is now implemented.
Dead config field — SessionTimeout int in config.go is loaded but never read by any code. The actual timeout uses SessionIdleTimeout string. Clean up the dead field or unify them.
## 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)
- `runCleanup` properly calls `cleanupUser` for each stale session before `DeleteStaleUsers`
- `cleanupUser` works correctly with `context.Context` — no regressions in HTTP handler path (`HandleLogout` passes `request.Context()`)
- QUIT notifications properly sent to all channel members (deduplicated via `notified` map)
- CASCADE on `clients.session_id` means `DeleteSession` in `cleanupUser` cascades to clients; subsequent `DeleteStaleUsers` harmlessly finds nothing — safe
- No concurrency issues: SQLite serializes writes, cleanup goroutine and HTTP handlers share the DB safely
- No cheating: no test changes, no linter config changes, no `Makefile` changes
- `docker build .` passes
### README: ❌ Not updated
The README must be updated to reflect the new functionality:
1. **`GET /api/v1/server` response** — [README line ~1168](https://git.eeqj.de/sneak/chat/src/branch/feature/mvp-remaining/README.md#L1168) shows `{"name": ..., "motd": ...}` but the code now returns `{"name": ..., "motd": ..., "users": ...}`. Add the `users` field.
2. **`POST /api/v1/logout` endpoint** — Not documented anywhere in the README. Needs its own section in the API Reference.
3. **`GET /api/v1/users/me` endpoint** — Not documented. Needs its own section (can note it returns the same response as `GET /api/v1/state`).
4. **Configuration table** — [README line ~1593](https://git.eeqj.de/sneak/chat/src/branch/feature/mvp-remaining/README.md#L1593) documents `SESSION_TIMEOUT` (int, seconds, "planned") but the actual implementation uses `SESSION_IDLE_TIMEOUT` (string, Go duration like `24h`). Either:
- Replace `SESSION_TIMEOUT` with `SESSION_IDLE_TIMEOUT` and update type/description, or
- Remove the dead `SessionTimeout` field from config and use `SESSION_IDLE_TIMEOUT` consistently
5. **Storage section** — [README line ~1575](https://git.eeqj.de/sneak/chat/src/branch/feature/mvp-remaining/README.md#L1575) says session expiry is "planned" — it is now implemented.
6. **Dead config field** — `SessionTimeout int` in `config.go` is loaded but never read by any code. The actual timeout uses `SessionIdleTimeout string`. Clean up the dead field or unify them.
- Document POST /api/v1/logout endpoint
- Document GET /api/v1/users/me endpoint
- Add 'users' field to GET /api/v1/server response docs
- Fix config: SESSION_TIMEOUT -> SESSION_IDLE_TIMEOUT
- Update storage section: session expiry is implemented
- Update roadmap: move session expiry to implemented
- Remove dead SessionTimeout config field from Go code
GET /api/v1/server response — added users field to documented response with field table
POST /api/v1/logout — added full API Reference section with request/response/errors/curl example
GET /api/v1/users/me — added full API Reference section (documents it as alias for GET /api/v1/state)
Config table — SESSION_TIMEOUT → SESSION_IDLE_TIMEOUT (type: string, default: 24h), also updated .env example
Storage section — session expiry updated from "planned" to implemented with description of cleanup behavior
Dead SessionTimeout config field — removed field, viper default, and viper read from internal/config/config.go
Also updated Roadmap: moved session expiry, logout, users/me, and user count to Implemented section.
make fmt✅ | docker build .✅ | All tests pass ✅
Rework complete. All 6 review findings addressed:
1. **`GET /api/v1/server` response** — added `users` field to documented response with field table
2. **`POST /api/v1/logout`** — added full API Reference section with request/response/errors/curl example
3. **`GET /api/v1/users/me`** — added full API Reference section (documents it as alias for `GET /api/v1/state`)
4. **Config table** — `SESSION_TIMEOUT` → `SESSION_IDLE_TIMEOUT` (type: string, default: `24h`), also updated `.env` example
5. **Storage section** — session expiry updated from "planned" to implemented with description of cleanup behavior
6. **Dead `SessionTimeout` config field** — removed field, viper default, and viper read from `internal/config/config.go`
Also updated Roadmap: moved session expiry, logout, users/me, and user count to Implemented section.
`make fmt` ✅ | `docker build .` ✅ | All tests pass ✅
✅ Storage section updated: session expiry is implemented
✅ Dead SessionTimeout config field removed from Go code
✅ Roadmap updated: implemented features moved out of planned
docker build . passes. No regressions from the dead config removal. PR is up to date with main.
## Re-review: PASS ✅
All 6 findings from the previous review are fixed:
1. ✅ `users` field documented in `GET /api/v1/server` response
2. ✅ `POST /api/v1/logout` endpoint fully documented
3. ✅ `GET /api/v1/users/me` endpoint fully documented
4. ✅ Config uses `SESSION_IDLE_TIMEOUT` (not `SESSION_TIMEOUT`)
5. ✅ Storage section updated: session expiry is implemented
6. ✅ Dead `SessionTimeout` config field removed from Go code
7. ✅ Roadmap updated: implemented features moved out of planned
`docker build .` passes. No regressions from the dead config removal. PR is up to date with main.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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.