Clear old localhost cookies when issuing new ones

[calvinstudebaker]

localhost cookie jar was getting too large

---

Note

Medium Risk
Changes local dev auth cookie lifecycle and lock-based sweeps; mistakes could clear active sessions or leave stale cookies, but scope is localhost dev only—not production auth.

Overview
Repeated wavedash dev runs mint new build UUIDs and new wd_*_<uuid> cookies that all share the localhost jar, so old cookies piled up and could bloat request headers.

Each dev server now holds a <uuid>.lock under dev-sessions for its lifetime (released on exit/crash). On the auth callback, the server scans incoming cookies, expires wd_session_ / wd_jwt_ / wd_sdkconfig_ names for builds that are not the current run and whose lock is absent or acquirable (dead server), and skips cookies for builds that still hold the lock (concurrent live servers).

New session cookies use shared set_cookie / expire_cookie helpers with 1-day Max-Age so orphaned sessions eventually age out even without another handoff. get_temp_credentials is refactored to take a BuildUploadInfo struct; local Claude settings are removed; a redundant total_bytes binding in the uploader is dropped.

^{Reviewed by Cursor Bugbot for commit 24856ff. Bugbot is set up for automated code reviews on this repo. Configure here.}

[claude]

Claude finished @calvinstudebaker's task in 1m 58s —— View job

---

Code review: Clear old localhost cookies when issuing new ones

Read the full diff and src/dev/server.rs. The refactor (centralizing cookie formatting into set_cookie/expire_cookie) is clean and the explanatory comments are excellent. One design concern worth a decision, plus a couple of minor notes.

🟠 Concurrent dev servers get swept (see inline comment)

stale_wavedash_cookies treats every wd_* cookie that isn't this build's three as stale. But cookies are host-scoped and ignore ports, so a second dev server running concurrently on another port shares the jar — and its live wd_* cookies look identical to orphaned ones. Completing a handoff on build B will expire build A's session.

This directly contradicts the build_uuid doc comment (lines 43–44): "to keep concurrent and successive dev servers from clobbering each other." Before this PR each server only wrote its own cookies and they coexisted; this adds cross-build deletion. Build A heals on its next reload, so it degrades rather than hard-breaks — but it undercuts the stated concurrency guarantee. Either relax that comment if concurrent servers are no longer supported, or distinguish live builds from orphans (e.g. a small lockfile/registry of active uuids the sweep consults).

🟡 Session cookies become persistent (1 day)

Adding Max-Age=86400 to set_cookie turns what were session cookies (cleared on browser close) into persistent ones surviving restarts. The comment on SESSION_MAX_AGE_SECS explains the intent (orphans self-expire even with no later handoff to sweep them), so this looks deliberate — flagging only so it's a conscious tradeoff: a leftover session now lingers up to a day on disk instead of dying with the browser.

🔵 Minor

• expire_cookie comment says "Path and attributes must match" — only name + Path + Domain matter for browser deletion matching; HttpOnly/SameSite are ignored. Harmless to keep them, but the comment slightly overstates the requirement.
• The sweep runs only in handle_callback's success path. That's the natural "one request that sees them all" hook and the Max-Age fallback covers the rest, so this is fine — just noting it isn't a guaranteed cleanup on every request.

✅ Verified fine

• urlencoding::encode(...).as_ref() → &str matches set_cookie's signature.
• Current build's three cookies are correctly excluded from the sweep (!current.iter().any(...)), so the new cookies and the expirations never conflict in the same response.
• split_once('=') for name extraction is safe (cookie names contain no =).
• expire_cookie matches the original Path=/ of swept cookies, so deletion takes effect (including for pre-PR session cookies that had no Max-Age).

Verdict: Solid, well-documented change that fixes the jar-overflow problem. The only thing I'd resolve before merge is the concurrent-server interaction — confirm it's acceptable or guard against it.

---

• Read the diff and full file
• Analyze correctness, edge cases, readability
• Post review

[claude]

This sweep breaks concurrently running dev servers, which the build-uuid scheme was explicitly designed to support.

The ServeConfig.build_uuid doc comment (lines 43–44) states the per-build cookie names exist "to keep concurrent and successive dev servers from clobbering each other." Cookies are host-scoped and ignore ports, so the browser replays every localhost server's wd_* cookies to this one.

stale_wavedash_cookies treats any wd_* cookie that isn't this build's three as stale — it can't distinguish "orphaned by a dead server" from "live, belonging to another dev server running right now on a different port." So if build A and build B run at the same time and you complete a handoff on B, B's callback expires A's wd_session_/wd_jwt_/wd_sdkconfig_ cookies, killing A's authenticated session.

Before this PR, concurrent servers coexisted (each only ever wrote its own uuid-named cookies); this introduces cross-build deletion. Build A recovers on its next reload (handoff re-runs), so it's a degradation rather than a hard break — but it does undercut the stated concurrency guarantee.

If concurrent dev servers are no longer a supported scenario, consider updating the lines 43–44 comment to say so. If they are still supported, the sweep needs a way to tell live builds apart from orphans — e.g. a small shared registry/lockfile of active build uuids that each server adds itself to on startup and removes on shutdown, so the sweep only expires uuids not in that set.

[claude]

Claude finished @calvinstudebaker's task in 3m 49s —— View job

---

Code review: Clear old localhost cookies when issuing new ones

Re-reviewed after the latest push (which removes .claude/settings.local.json from version control — good cleanup, no issues there). Read the full diff and src/dev/server.rs. The refactor — centralizing cookie set/clear into set_cookie/expire_cookie — is clean and the explanatory comments are genuinely excellent. The change compiles (cargo check passes). One design concern remains worth resolving before merge, plus a couple of minor notes.

🟠 Concurrent dev servers still get swept (open thread on the sweep)

stale_wavedash_cookies treats every wd_session_/wd_jwt_/wd_sdkconfig_ cookie that isn't this build's three as stale. But cookies are host-scoped and ignore ports, so a second wavedash dev running concurrently on another port shares the jar — and its live wd_* cookies are indistinguishable from orphaned ones. Completing a handoff on build B will expire build A's session.

This directly contradicts the build_uuid doc comment (src/dev/server.rs:43-44): "to keep concurrent and successive dev servers from clobbering each other." Before this PR each server only ever wrote its own uuid-named cookies, so they coexisted; the sweep adds cross-build deletion. Build A self-heals on its next reload (the handoff re-runs), so this degrades rather than hard-breaks — but it undercuts the stated concurrency guarantee.

Two clean ways forward:

• If concurrent dev servers are no longer a supported scenario, relax the :43-44 comment to say so.
• If they're still supported, give the sweep a way to tell live builds from orphans — e.g. a small lockfile/registry of active build uuids each server adds itself to on startup and removes on shutdown, which the sweep consults before expiring.

Discuss/fix →

🟡 Session cookies are now persistent (1 day)

Adding Max-Age=86400 in set_cookie turns what were session cookies (cleared on browser close) into persistent ones that survive restarts. The SESSION_MAX_AGE_SECS comment explains the intent — an orphan with no later handoff to sweep it still self-expires — so this reads as deliberate. Flagging only so the tradeoff is conscious: a leftover dev session now lingers on disk up to a day instead of dying with the browser.

🔵 Minor

• expire_cookie's doc comment slightly overstates the deletion-matching rule — left an inline suggestion. Only name + Path + Domain matter; HttpOnly/SameSite are ignored.
• The sweep runs only in handle_callback's success path. That's the natural "one request that sees them all" hook, and the Max-Age fallback covers the rest, so this is fine — just noting it isn't a guaranteed cleanup on every request.

✅ Verified fine

• Compiles cleanly (cargo check).
• urlencoding::encode(...).as_ref() → &str matches set_cookie's signature.
• This build's three cookies are correctly excluded from the sweep (!current.iter().any(...)), so the new sets and the expirations never collide in the same response.
• split_once('=') for name extraction is safe (cookie names contain no =).
• expire_cookie matches the swept cookies' original Path=/ (including pre-PR session cookies that had no Max-Age), so deletion actually takes effect.
• All cookie writes now route through set_cookie/expire_cookie — no stragglers left using the old inline format!.

Verdict: Solid, well-documented change that fixes the jar-overflow problem. The only thing I'd resolve before merge is the concurrent-server interaction — confirm it's acceptable (and relax the :43-44 comment) or guard against it.

---

• Read the diff and full file
• Verify it compiles (cargo check passes)
• Analyze correctness, edge cases, readability
• Post inline comment + review summary
  • Branch: cleanup-stale-cookies

[claude]

🔵 Minor (comment accuracy): per RFC 6265 a cookie is identified for replacement by name + Path + Domain only — HttpOnly, SameSite, value and Max-Age play no part in the match. So "Path and attributes must match" slightly overstates it: keeping the attributes here is harmless, but it's the matching Path=/ (and absent Domain, same as set_cookie) that actually makes the browser drop the cookie.

Suggested change

	/// The deletion form (empty value, immediate expiry) of `set_cookie`; Path and
	/// attributes must match for the browser to drop it.
	/// The deletion form (empty value, immediate expiry) of `set_cookie`. A cookie
	/// is matched for replacement by name + Path + Domain only, so the shared
	/// `Path=/` (and no Domain) is what makes the browser drop it.

[claude]

Claude finished @calvinstudebaker's task in 2m 16s —— View job

---

Code review: Clear old localhost cookies when issuing new ones

Re-reviewed after the latest push (24856ff — "support concurrent wavedash dev local servers, cookies cleared if no server has a lock"). This commit directly addresses the concurrency concern raised in the prior two rounds. Read the full diff and src/dev/{mod,server}.rs.

✅ The concurrent-server concern is resolved

The earlier worry — that stale_wavedash_cookies would sweep a live concurrent dev server's cookies (cookies are host-scoped and ignore ports) — is now handled correctly:

• Each wavedash dev acquires an exclusive lock on ~/.wavedash/dev-sessions/<uuid>.lock at startup (src/dev/mod.rs:144-160) and holds it for the process lifetime. The _session_lock binding lives until the end of handle_dev, and server::run(...).await is the final statement in scope, so the lock is held for the whole server lifetime. ✔
• build_is_live (src/dev/server.rs:316) probes that lock before expiring any build's cookies: a held lock → WouldBlock → kept (live sibling); a free/missing lock → swept (orphan), and the dead lockfile is opportunistically removed. ✔
• Liveness is cached per-uuid in the sweep so the three cookies of one build cost a single lock probe. ✔
• OS releases the lock on clean exit or crash — exactly when leftovers become safe to expire — so the build_uuid "concurrent and successive servers" guarantee at :43-46 now actually holds. ✔

This is a clean, correct fix.

🔵 Minor — open-failure path is more permissive than the lock-failure path

build_is_live treats any open error as "exited" (src/dev/server.rs:318-320), while the try_lock arms deliberately treat errors as "live" ("never sweep a build we can't prove is gone"). A non-NotFound open failure would sweep a possibly-live build — the opposite of that stated principle. Near-impossible for local user-owned files, so very low severity; left an inline suggestion to distinguish NotFound if you want the two paths consistent.

🔵 Minor — own lockfile isn't removed on clean exit

A server never deletes its own <uuid>.lock; cleanup relies on a future server's build_is_live probe. Run wavedash dev once and never again and the empty lockfile lingers in dev-sessions/. Cosmetic (empty files, opportunistically reaped) — just noting the dir can slowly accumulate orphan lockfiles in the no-future-runs case.

🔵 Pre-existing nits (carried over, optional)

• expire_cookie's doc comment (:262) still says "Path and attributes must match" — only name + Path + Domain matter for the browser's replace match. Harmless.
• Max-Age=86400 turns the session cookies persistent (survive browser close). The SESSION_MAX_AGE_SECS comment shows this is deliberate (orphans self-expire even with no later sweep) — flagging only so it stays a conscious tradeoff.

✅ Also verified

• src/builds.rs: the get_temp_credentials arg-list → BuildUploadInfo struct refactor is purely mechanical; every field maps 1:1, no behavior change.
• src/uploader.rs: removing the no-op let total_bytes = total_bytes; self-rebind is correct.
• All cookie writes route through set_cookie/expire_cookie — no inline format! stragglers; the current build's three cookies are still excluded from the sweep, so sets and expirations never collide in one response.
• File::try_lock/std::fs::TryLockError is stable std (1.89+); prior rounds confirmed cargo check passes.

Verdict: The blocking concern from the previous reviews is resolved — the lockfile registry correctly distinguishes live concurrent servers from orphans, and the rest of the change is solid and well-documented. Remaining items are all minor/optional. LGTM. 👍

---

• Read the full diff and changed files
• Analyze the new concurrent-server lock mechanism
• Verify correctness / edge cases
• Post review
  • Branch: cleanup-stale-cookies

[claude]

🔵 Minor (defensive consistency): the open path here treats any error as "exited" → false, which sweeps that build's cookies. But the try_lock arms below are deliberately conservative (TryLockError::Error(_) => true, "never sweep a build we can't prove is gone"). So a non-NotFound open failure (e.g. a transient EACCES/FS error) would sweep a build that might still be live — the opposite of the stated principle.

In practice these are files the same user just created on local disk, so a non-NotFound error is near-impossible and this is very low severity. If you want the two paths to agree, distinguish the not-found case:

let file = match std::fs::OpenOptions::new().read(true).open(&path) {
    Ok(f) => f,
    Err(e) if e.kind() == std::io::ErrorKind::NotFound => return false, // owner exited, lockfile gone
    Err(_) => return true, // can't prove it's gone → keep, same as the lock-error arm
};

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 24856ff. Configure here.}

[cursor]

Missing lock treats live server dead

Medium Severity

build_is_live returns dead when the session lock file cannot be opened, but a dev server can still be running without that file (older CLI without locks, or the lock file removed while the process keeps its handle). Another localhost dev auth callback may then expire that build’s wd_* cookies and break its session.

 

^{Reviewed by Cursor Bugbot for commit 24856ff. Configure here.}