toes/ISSUES.md

# Issues - Versioned Deployment Implementation

## Critical Issues

### 1. Watch Filter Breaks File Change Detection

**Location**: `src/server/apps.ts:589-593`

**Problem**: The watch logic ignores all changes deeper than 2 levels:

```ts
// Ignore changes deeper than 2 levels (inside timestamp dirs)
if (parts.length > 2) return

// For versioned apps, only care about changes to "current" directory
if (parts.length === 2 && parts[1] !== 'current' && parts[1] !== 'package.json') return
```

Files inside `current/` are 3 levels deep: `appname/current/somefile.ts`

This **ignores all file changes** inside the current directory, breaking hot reload and auto-restart.

**Fix**:
```ts
// Ignore changes inside old timestamp dirs (but allow current/)
if (parts.length > 2 && parts[1] !== 'current') return
```

---

### 2. App Restart Race Condition

**Location**: `src/server/api/sync.ts:145-148`

**Problem**: Activation uses arbitrary 1 second delay without confirming stop completed:

```ts
// Restart app to use new version
const app = allApps().find(a => a.name === appName)
if (app?.state === 'running') {
  stopApp(appName)
  setTimeout(() => startApp(appName), 1000)  // ⚠️ Arbitrary 1s delay
}
```

**Issues**:
- 1 second may not be enough for app to fully stop
- No confirmation that stop completed before start
- If app crashes during startup, activation still returns success

**Fix**: Make activation async and wait for stop to complete, or move restart logic to after symlink succeeds and poll for stop completion.

---

## Major Issues

### 3. No Version Cleanup

**Problem**: Old timestamp directories accumulate forever with no cleanup mechanism.

**Impact**: Disk space grows indefinitely as deployments pile up.

**Recommendation**: Add cleanup logic:
- Keep last N versions (e.g., 5-10)
- Delete versions older than X days
- Expose as `toes clean <app>` command or automatic post-activation

---

### 4. safePath Behavior Change

**Location**: `src/server/api/sync.ts:14-21`

**Problem**: Security model changed by resolving symlinks in base path:

```ts
const canonicalBase = existsSync(base) ? realpathSync(base) : base
```

Previously paths were checked against literal base, now against resolved base. This changes behavior if someone creates a symlink attack.

**Recommendation**: Document this intentional change, or keep original check and add separate symlink resolution only where needed.

---

### 5. Non-Atomic Deploy Copy

**Location**: `src/server/api/sync.ts:133-141`

**Problem**: Race condition possible during deploy:

```ts
if (existsSync(currentLink)) {
  const currentReal = realpathSync(currentLink)
  cpSync(currentReal, newVersion, { recursive: true })
}
```

If `current` changes between `existsSync` and `cpSync`, stale code might be copied.

**Impact**: Low probability for single-user system, but possible during concurrent deploys.

**Fix**: Read symlink once and reuse: `const currentReal = existsSync(currentLink) ? realpathSync(currentLink) : null`

---

### 6. Upload Error Handling Inconsistency

**Location**: `src/cli/commands/sync.ts:113-115`

**Problem**: Upload continues if a file fails but doesn't track failures:

```ts
if (success) {
  console.log(`  ${color.green('↑')} ${file}`)
} else {
  console.log(`  ${color.red('✗')} ${file}`)
  // Continue even if one file fails
}
```

If any file fails to upload, deployment still activates with incomplete files.

**Fix**: Either:
- Abort entire deployment on first failure
- Track failures and warn/abort before activating

---

## Minor Issues

### 7. POST Body Check Too Loose

**Location**: `src/cli/http.ts:42-44`

**Problem**: Falsy values like `0`, `false`, or `""` would incorrectly skip body:

```ts
headers: body ? { 'Content-Type': 'application/json' } : undefined,
body: body ? JSON.stringify(body) : undefined,
```

**Fix**:
```ts
headers: body !== undefined ? { 'Content-Type': 'application/json' } : undefined,
body: body !== undefined ? JSON.stringify(body) : undefined,
```

---

### 8. Unconventional Timestamp Format

**Location**: `src/server/api/sync.ts:115-123`

**Problem**: Unusual array construction with separator as element:

```ts
const timestamp = [
  now.getFullYear(),
  String(now.getMonth() + 1).padStart(2, '0'),
  String(now.getDate()).padStart(2, '0'),
  '-',  // Unusual to put separator as array element
  String(now.getHours()).padStart(2, '0'),
  String(now.getMinutes()).padStart(2, '0'),
  String(now.getSeconds()).padStart(2, '0'),
].join('')
```

**Recommendation**: Use template literal:
```ts
const pad = (n: number) => String(n).padStart(2, '0')
const timestamp = `${now.getFullYear()}${pad(now.getMonth()+1)}${pad(now.getDate())}-${pad(now.getHours())}${pad(now.getMinutes())}${pad(now.getSeconds())}`
```

Or ISO format: `now.toISOString().replace(/[:.]/g, '-').slice(0, -5)`

---

## Positive Points

✓ Atomic symlink swapping with temp file pattern is correct
✓ Clear 3-step deployment protocol (deploy, upload, activate)
✓ Proper use of `realpathSync` to resolve canonical paths for running apps
✓ Good separation of concerns in API routes

---

## Priority

1. **BLOCKER**: Fix watch filter (#1) - breaks hot reload
2. **HIGH**: Fix restart race condition (#2) - affects deployment reliability
3. **HIGH**: Add version cleanup (#3) - disk space concern
4. **MEDIUM**: Fix upload error handling (#6) - data integrity
5. **LOW**: Everything else