Freebuff waiting room

[jahooma]

No description provided.

[greptile-apps]

Greptile Summary

This PR introduces a freebuff waiting room — a queue-based admission system that gates free-tier CLI users before allowing them to send chat requests. It is implemented end-to-end across the server (new free_session table, admission ticker, /api/v1/freebuff/session REST endpoints, and a gate injected into the chat-completions hot path) and the CLI (a polling hook useFreebuffSession, zustand store, and two new terminal UI screens for the waiting room and the "superseded" terminal state).

Key design decisions:

• Feature-flagged off by default (FREEBUFF_WAITING_ROOM_ENABLED=false), requiring explicit opt-in per environment.
• Advisory lock + FOR UPDATE SKIP LOCKED in admitFromQueue prevents double-admission across pods.
• Instance ID rotation on every POST /session enforces one-CLI-per-account.
• Dependency injection throughout server code makes the layer fully unit-testable without DB.

Notable issues found:

• No TTL/expiry on queued rows: If a CLI crashes without sending the cleanup DELETE, its queued row stays in the table indefinitely, inflating queue depth and wait-time estimates for all other users.
• Null claimedInstanceId → session_superseded: A missing instance ID in an active-session request returns the same 409 code as a genuine takeover, making the two conditions indistinguishable in logs.
• Non-transactional position + depth reads: queuePositionFor and queueDepth() run in parallel without a transaction, so position can transiently exceed depth on the client display.
• onDisableAds={() => {}} in WaitingRoomScreen: The no-op handler may render a dead affordance if AdBanner shows a disable button.

Confidence Score: 4/5

Safe to merge behind the feature flag; the queued-row TTL gap is real but won't manifest until the feature is enabled in production.

The implementation is thorough and well-tested with dependency injection throughout. The feature defaults to OFF so no existing request path is affected. The main gap — no TTL on queued rows — is a correctness concern once the feature goes live but does not break the primary admission or chat path. The two P2 issues are minor observability/cosmetic concerns.

web/src/server/free-session/store.ts (queued-row TTL), web/src/server/free-session/public-api.ts (null instanceId path and non-transactional reads)

Important Files Changed

Filename	Overview
web/src/server/free-session/store.ts	Core DB layer; uses advisory lock + FOR UPDATE SKIP LOCKED for safe concurrent admission. Main concern: no TTL on queued rows means stale entries accumulate indefinitely if CLIs crash without sending DELETE.
web/src/server/free-session/public-api.ts	Session gate and state API; null claimedInstanceId is treated as session_superseded (same 409 as a rotated instance), conflating missing-field with takeover. Position + depth queries are non-transactional.
web/src/server/free-session/admission.ts	Background admission ticker with overlap-prevention (inFlight guard) and Fireworks health gate. Well-structured with injectable deps for testing.
cli/src/hooks/use-freebuff-session.ts	Polling hook managing POST/GET/DELETE lifecycle and bell-on-admission. Module-level activeRefreshHandle enables imperative refresh from gate-error handlers. Cleanup is well-guarded.
cli/src/components/waiting-room-screen.tsx	Waiting room UI with elapsed timer, position display, and ad banner. onDisableAds is a silent no-op — acceptable if AdBanner hides the affordance, but worth verifying.
web/src/app/api/v1/chat/completions/_post.ts	Gate check injected before rate-limiter; only runs for free-mode requests when waiting room is enabled. Correctly returns structured 4xx by error code.
cli/src/app.tsx	AuthedSurface wrapper correctly gates WaitingRoomScreen and FreebuffSupersededScreen before rendering Chat.
packages/internal/src/db/schema.ts	New free_session table with correct PK (user_id), cascade FK, and two targeted indexes for queue dequeue and expiry sweep.
web/src/server/free-session/session-view.ts	Pure view-projection function with injected clock; well-tested. estimateWaitMs uses wave-based upper-bound formula.
cli/src/hooks/helpers/send-message.ts	Gate errors mapped to user-readable messages; superseded is treated as terminal, others trigger a forcePost refresh.

Sequence Diagram

sequenceDiagram
    participant CLI
    participant SessionAPI as /api/v1/freebuff/session
    participant ChatAPI as /api/v1/chat/completions
    participant DB as free_session (Postgres)
    participant Ticker as AdmissionTicker

    CLI->>SessionAPI: POST /session (on startup)
    SessionAPI->>DB: joinOrTakeOver (UPSERT)
    DB-->>SessionAPI: row {status: queued, instanceId}
    SessionAPI-->>CLI: {status: queued, position, estimatedWaitMs}

    loop Poll every 5s while queued
        CLI->>SessionAPI: GET /session
        SessionAPI->>DB: getSessionRow
        DB-->>SessionAPI: row
        SessionAPI-->>CLI: {status: queued | active}
    end

    Ticker->>DB: sweepExpired() + admitFromQueue() every 5s
    Note over Ticker,DB: advisory lock prevents double-admission across pods
    DB-->>Ticker: admitted rows
    Note over CLI: bell plays on queued→active transition

    CLI->>ChatAPI: POST /completions + freebuff_instance_id
    ChatAPI->>DB: checkSessionAdmissible(userId, instanceId)
    alt session active and instance matches
        DB-->>ChatAPI: {ok: true}
        ChatAPI-->>CLI: 200 stream
    else queued / expired / superseded
        DB-->>ChatAPI: {ok: false, code}
        ChatAPI-->>CLI: 428/429/409/410
        CLI->>SessionAPI: refreshFreebuffSession() or markSuperseded()
    end

    CLI->>SessionAPI: DELETE /session (on exit)
    SessionAPI->>DB: endSession(userId)

Loading

Comments Outside Diff (1)

1. cli/src/components/waiting-room-screen.tsx, line 386-388 (link)

   onDisableAds is a no-op — may render a dead UI affordance

   onDisableAds={() => {}} passes a silent no-op to AdBanner. If AdBanner renders any "Disable ads" affordance based on this prop, clicking it will silently do nothing. Consider passing undefined if AdBanner hides the affordance in that case, or add a comment explaining why no-op is intentional here (e.g. "free users cannot disable ads in the waiting room").

_{Reviews (1): Last reviewed commit: "Merge branch 'main' into freebuff-waitin..." | Re-trigger Greptile}

[greptile-apps]

No TTL on queued rows — stale entries accumulate indefinitely

sweepExpired only removes rows where status = 'active' AND expires_at < now. Queued rows are never automatically removed. A user who joins the queue (POST /session) and then hard-crashes or loses network before their CLI can send the fire-and-forget DELETE will leave a stale status=queued row in the table forever. These phantom entries:

1. Inflate queueDepth() for all other users, making wait-time estimates wrong.
2. Keep would-be new users at artificially higher positions until a real admin runs cleanup.

Consider adding a queued_at + max_queue_wait expiry sweep alongside the active-session sweep, e.g.:

// In sweepExpired (store.ts): also remove queued rows idle for too long
await db.delete(schema.freeSession).where(
  and(
    eq(schema.freeSession.status, 'queued'),
    lt(schema.freeSession.queued_at, new Date(now.getTime() - MAX_QUEUE_IDLE_MS)),
  ),
)

Alternatively, track the last heartbeat time and sweep queued rows that haven't polled in N minutes.

[greptile-apps]

Null claimedInstanceId returns session_superseded instead of a distinct error

When claimedInstanceId is null or undefined (absent from the request), the code falls through to the session_superseded branch with the message "Another instance of freebuff has taken over this session." This conflates two separate conditions — a missing identifier vs. a rotated one — under the same 409 code.

In the current UI flow this is safe (the WaitingRoomScreen gate prevents users from sending messages before their instance ID is populated), but it makes server-side observability harder and would produce a confusing 409 for any future client that omits the field.

Consider splitting the two cases:

if (!params.claimedInstanceId) {
  return {
    ok: false,
    code: 'waiting_room_required',
    message: 'freebuff_instance_id missing from request. Call POST /api/v1/freebuff/session first.',
  }
}
if (params.claimedInstanceId !== row.active_instance_id) {
  return { ok: false, code: 'session_superseded', message: '...' }
}

[greptile-apps]

queuePositionFor and queueDepth are not fetched in a transaction

These two queries run in parallel outside a transaction. If users are admitted between the two reads, position could exceed depth (e.g. position=3, depth=2), which would be confusing on the client's waiting-room display. This is cosmetic rather than a correctness bug, but wrapping both in a single read-only transaction or a WITH CTE would eliminate the inconsistency.