Skip to content

Commit 9b1f388

Browse files
feat(stability): add 4 critical stability improvements for 1.0.0
1. Replace Linear sync boolean lock with AsyncMutex - Prevents race conditions during concurrent sync attempts - Includes stale lock detection (5 min timeout) - Thread-safe acquire/tryAcquire/release pattern 2. Add timeout to SMS action execution - 60 second timeout prevents hanging requests - Uses Promise.race pattern for clean timeout handling 3. Persist rate limiting to disk - Rate limits now survive server restarts - Uses in-memory cache with periodic persistence (30s) - Auto-cleanup of expired entries on load 4. Atomic write pattern for JSON files - writeFileSecure now writes to temp file first, then renames - Prevents file corruption on crash/interrupt - Cleanup of temp files on failure
1 parent 25efe8e commit 9b1f388

5 files changed

Lines changed: 283 additions & 39 deletions

File tree

src/core/utils/async-mutex.ts

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
/**
2+
* Simple async mutex implementation for thread-safe operations
3+
* Prevents race conditions when multiple async operations compete for the same resource
4+
*/
5+
6+
export class AsyncMutex {
7+
private locked = false;
8+
private waiting: Array<() => void> = [];
9+
private lockHolder: string | null = null;
10+
private lockAcquiredAt: number = 0;
11+
private readonly lockTimeout: number;
12+
13+
constructor(lockTimeoutMs: number = 300000) {
14+
// Default 5 minute timeout
15+
this.lockTimeout = lockTimeoutMs;
16+
}
17+
18+
/**
19+
* Acquire the lock. Waits if already locked.
20+
* Returns a release function that MUST be called when done.
21+
*/
22+
async acquire(holder?: string): Promise<() => void> {
23+
// Check for stale lock (lock timeout)
24+
if (this.locked && this.lockAcquiredAt > 0) {
25+
const elapsed = Date.now() - this.lockAcquiredAt;
26+
if (elapsed > this.lockTimeout) {
27+
console.warn(
28+
`[AsyncMutex] Stale lock detected (held by ${this.lockHolder} for ${elapsed}ms), forcing release`
29+
);
30+
this.forceRelease();
31+
}
32+
}
33+
34+
if (!this.locked) {
35+
this.locked = true;
36+
this.lockHolder = holder || 'unknown';
37+
this.lockAcquiredAt = Date.now();
38+
return () => this.release();
39+
}
40+
41+
// Wait for lock to be released
42+
return new Promise((resolve) => {
43+
this.waiting.push(() => {
44+
this.locked = true;
45+
this.lockHolder = holder || 'unknown';
46+
this.lockAcquiredAt = Date.now();
47+
resolve(() => this.release());
48+
});
49+
});
50+
}
51+
52+
/**
53+
* Try to acquire the lock without waiting
54+
* Returns release function if acquired, null if already locked
55+
*/
56+
tryAcquire(holder?: string): (() => void) | null {
57+
// Check for stale lock
58+
if (this.locked && this.lockAcquiredAt > 0) {
59+
const elapsed = Date.now() - this.lockAcquiredAt;
60+
if (elapsed > this.lockTimeout) {
61+
console.warn(
62+
`[AsyncMutex] Stale lock detected (held by ${this.lockHolder} for ${elapsed}ms), forcing release`
63+
);
64+
this.forceRelease();
65+
}
66+
}
67+
68+
if (!this.locked) {
69+
this.locked = true;
70+
this.lockHolder = holder || 'unknown';
71+
this.lockAcquiredAt = Date.now();
72+
return () => this.release();
73+
}
74+
return null;
75+
}
76+
77+
private release(): void {
78+
const next = this.waiting.shift();
79+
if (next) {
80+
next();
81+
} else {
82+
this.locked = false;
83+
this.lockHolder = null;
84+
this.lockAcquiredAt = 0;
85+
}
86+
}
87+
88+
private forceRelease(): void {
89+
this.locked = false;
90+
this.lockHolder = null;
91+
this.lockAcquiredAt = 0;
92+
}
93+
94+
/**
95+
* Execute a function while holding the lock
96+
*/
97+
async withLock<T>(fn: () => Promise<T>, holder?: string): Promise<T> {
98+
const release = await this.acquire(holder);
99+
try {
100+
return await fn();
101+
} finally {
102+
release();
103+
}
104+
}
105+
106+
/**
107+
* Check if currently locked
108+
*/
109+
isLocked(): boolean {
110+
return this.locked;
111+
}
112+
113+
/**
114+
* Get current lock status
115+
*/
116+
getStatus(): {
117+
locked: boolean;
118+
holder: string | null;
119+
acquiredAt: number;
120+
waitingCount: number;
121+
} {
122+
return {
123+
locked: this.locked,
124+
holder: this.lockHolder,
125+
acquiredAt: this.lockAcquiredAt,
126+
waitingCount: this.waiting.length,
127+
};
128+
}
129+
}
130+
131+
// Singleton instance for sync operations
132+
export const syncMutex = new AsyncMutex(300000); // 5 minute timeout

src/hooks/secure-fs.ts

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,21 @@
33
* Ensures config files have restricted permissions (0600)
44
*/
55

6-
import { writeFileSync, mkdirSync, chmodSync, existsSync } from 'fs';
7-
import { dirname } from 'path';
6+
import {
7+
writeFileSync,
8+
mkdirSync,
9+
chmodSync,
10+
existsSync,
11+
renameSync,
12+
unlinkSync,
13+
} from 'fs';
14+
import { dirname, join } from 'path';
15+
import { randomBytes } from 'crypto';
816

917
/**
1018
* Write file with secure permissions (0600 - user read/write only)
11-
* Also ensures parent directory has 0700 permissions
19+
* Uses atomic write pattern: write to temp file, then rename
20+
* This prevents corruption if process crashes mid-write
1221
*/
1322
export function writeFileSecure(filePath: string, data: string): void {
1423
const dir = dirname(filePath);
@@ -18,11 +27,29 @@ export function writeFileSecure(filePath: string, data: string): void {
1827
mkdirSync(dir, { recursive: true, mode: 0o700 });
1928
}
2029

21-
// Write file
22-
writeFileSync(filePath, data);
30+
// Generate temp file path in same directory (required for atomic rename)
31+
const tempPath = join(dir, `.tmp-${randomBytes(8).toString('hex')}`);
2332

24-
// Set secure permissions (user read/write only)
25-
chmodSync(filePath, 0o600);
33+
try {
34+
// Write to temp file first
35+
writeFileSync(tempPath, data);
36+
37+
// Set secure permissions on temp file
38+
chmodSync(tempPath, 0o600);
39+
40+
// Atomic rename (same filesystem, so this is atomic on POSIX)
41+
renameSync(tempPath, filePath);
42+
} catch (error) {
43+
// Clean up temp file on failure
44+
try {
45+
if (existsSync(tempPath)) {
46+
unlinkSync(tempPath);
47+
}
48+
} catch {
49+
// Ignore cleanup errors
50+
}
51+
throw error;
52+
}
2653
}
2754

2855
/**

src/hooks/sms-action-runner.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,12 @@ function isActionAllowed(action: string): boolean {
141141
});
142142
}
143143

144+
export interface ActionResult {
145+
success: boolean;
146+
output?: string;
147+
error?: string;
148+
}
149+
144150
export interface PendingAction {
145151
id: string;
146152
promptId: string;

src/hooks/sms-webhook.ts

Lines changed: 83 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import {
2727
queueAction,
2828
executeActionSafe,
2929
cleanupOldActions,
30+
type ActionResult,
3031
} from './sms-action-runner.js';
3132
import {
3233
isCommand,
@@ -56,16 +57,91 @@ const MAX_PHONE_LENGTH = 50; // WhatsApp format: whatsapp:+12345678901
5657
const MAX_BODY_SIZE = 50 * 1024; // 50KB max body
5758
const RATE_LIMIT_WINDOW_MS = 60 * 1000; // 1 minute
5859
const RATE_LIMIT_MAX_REQUESTS = 30; // 30 requests per minute per IP
60+
const ACTION_TIMEOUT_MS = 60000; // 60 second timeout for action execution
5961

60-
// Rate limiting store (in production, use Redis)
61-
const rateLimitStore = new Map<string, { count: number; resetTime: number }>();
62+
/**
63+
* Execute action with timeout to prevent hanging requests
64+
*/
65+
async function executeActionWithTimeout(
66+
action: string,
67+
response: string
68+
): Promise<ActionResult> {
69+
return Promise.race([
70+
executeActionSafe(action, response),
71+
new Promise<ActionResult>((_, reject) =>
72+
setTimeout(
73+
() =>
74+
reject(new Error(`Action timed out after ${ACTION_TIMEOUT_MS}ms`)),
75+
ACTION_TIMEOUT_MS
76+
)
77+
),
78+
]).catch((error) => ({
79+
success: false,
80+
error: error instanceof Error ? error.message : String(error),
81+
}));
82+
}
83+
84+
// Rate limiting store - persisted to disk to survive restarts
85+
const RATE_LIMIT_PATH = join(homedir(), '.stackmemory', 'rate-limits.json');
86+
87+
interface RateLimitRecord {
88+
count: number;
89+
resetTime: number;
90+
}
91+
92+
interface RateLimitStore {
93+
[ip: string]: RateLimitRecord;
94+
}
95+
96+
// In-memory cache with periodic persistence
97+
let rateLimitCache: RateLimitStore = {};
98+
let rateLimitCacheDirty = false;
99+
100+
function loadRateLimits(): RateLimitStore {
101+
try {
102+
if (existsSync(RATE_LIMIT_PATH)) {
103+
const data = JSON.parse(readFileSync(RATE_LIMIT_PATH, 'utf8'));
104+
// Clean up expired entries on load
105+
const now = Date.now();
106+
const cleaned: RateLimitStore = {};
107+
for (const [ip, record] of Object.entries(data)) {
108+
const r = record as RateLimitRecord;
109+
if (r.resetTime > now) {
110+
cleaned[ip] = r;
111+
}
112+
}
113+
return cleaned;
114+
}
115+
} catch {
116+
// Use empty store on error
117+
}
118+
return {};
119+
}
120+
121+
function saveRateLimits(): void {
122+
if (!rateLimitCacheDirty) return;
123+
try {
124+
ensureSecureDir(join(homedir(), '.stackmemory'));
125+
writeFileSecure(RATE_LIMIT_PATH, JSON.stringify(rateLimitCache));
126+
rateLimitCacheDirty = false;
127+
} catch {
128+
// Ignore save errors - rate limiting is best-effort
129+
}
130+
}
131+
132+
// Persist rate limits periodically (every 30 seconds)
133+
setInterval(saveRateLimits, 30000);
134+
135+
// Load on startup
136+
rateLimitCache = loadRateLimits();
62137

63138
function checkRateLimit(ip: string): boolean {
64139
const now = Date.now();
65-
const record = rateLimitStore.get(ip);
140+
const record = rateLimitCache[ip];
66141

67142
if (!record || now > record.resetTime) {
68-
rateLimitStore.set(ip, { count: 1, resetTime: now + RATE_LIMIT_WINDOW_MS });
143+
rateLimitCache[ip] = { count: 1, resetTime: now + RATE_LIMIT_WINDOW_MS };
144+
rateLimitCacheDirty = true;
69145
return true;
70146
}
71147

@@ -74,6 +150,7 @@ function checkRateLimit(ip: string): boolean {
74150
}
75151

76152
record.count++;
153+
rateLimitCacheDirty = true;
77154
return true;
78155
}
79156

@@ -298,11 +375,11 @@ export async function handleSMSWebhook(payload: TwilioWebhookPayload): Promise<{
298375
// Trigger notification to alert user/Claude
299376
triggerResponseNotification(result.response || Body);
300377

301-
// Execute action safely if present (no shell injection)
378+
// Execute action safely if present (no shell injection, with timeout)
302379
if (result.action) {
303380
console.log(`[sms-webhook] Executing action: ${result.action}`);
304381

305-
const actionResult = await executeActionSafe(
382+
const actionResult = await executeActionWithTimeout(
306383
result.action,
307384
result.response || Body
308385
);

0 commit comments

Comments
 (0)