Skip to content

Fix: bug(gateguard): race condition in session state read-modify-write path #1442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shahyashish wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: bug(gateguard): race condition in session state read-modify-write path #1442

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions scripts/hooks/gateguard-fact-force.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,37 @@ function pruneCheckedEntries(checked) {

function saveState(state) {
try {
state.last_active = Date.now();
state.checked = pruneCheckedEntries(state.checked);
fs.mkdirSync(STATE_DIR, { recursive: true });
// Atomic write: temp file + rename prevents partial reads
const tmpFile = STATE_FILE + '.tmp.' + process.pid;
fs.writeFileSync(tmpFile, JSON.stringify(state, null, 2), 'utf8');

// 1. Prepare base state from what's currently in memory
let mergedChecked = state.checked || [];
let mergedLastActive = state.last_active || 0;

// 2. Merge with current disk state to prevent overwriting concurrent updates (Option 2: Merge-style saves)
try {
if (fs.existsSync(STATE_FILE)) {
const disk = JSON.parse(fs.readFileSync(STATE_FILE, 'utf8'));
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Apr 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Concurrent saveState calls can still lose updates because merge-style read-modify-write is not synchronized across processes.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/gateguard-fact-force.js, line 88:

<comment>Concurrent saveState calls can still lose updates because merge-style read-modify-write is not synchronized across processes.</comment>

<file context>
@@ -76,14 +76,37 @@ function pruneCheckedEntries(checked) {
+    // 2. Merge with current disk state to prevent overwriting concurrent updates (Option 2: Merge-style saves)
+    try {
+      if (fs.existsSync(STATE_FILE)) {
+        const disk = JSON.parse(fs.readFileSync(STATE_FILE, 'utf8'));
+        if (Array.isArray(disk.checked)) {
+          // Union of checked items, preserving order (disk items first, then memory items)
</file context>
Fix with Cubic

if (Array.isArray(disk.checked)) {
// Union of checked items, preserving order (disk items first, then memory items)
mergedChecked = Array.from(new Set([...disk.checked, ...mergedChecked]));
}
if (typeof disk.last_active === 'number') {
mergedLastActive = Math.max(mergedLastActive, disk.last_active);
}
}
} catch (_) { /* ignore disk read errors, proceed with memory state */ }

// 3. Finalize state: update heartbeat and prune to bounded size
const finalState = {
checked: pruneCheckedEntries(mergedChecked),
last_active: Math.max(mergedLastActive, Date.now())
};

// 4. Atomic write: temp file + rename prevents partial reads
const tmpFile = STATE_FILE + '.tmp.' + process.pid + '.' + Math.random().toString(36).slice(2, 6);
fs.writeFileSync(tmpFile, JSON.stringify(finalState, null, 2), 'utf8');
fs.renameSync(tmpFile, STATE_FILE);
Comment on lines +86 to 108
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 TOCTOU window still present between merge-read and rename

The merge read (readFileSync on line 88) and the atomic rename (line 108) are separate, non-atomic operations. If two processes both complete their readFileSync before either calls renameSync, the second rename will silently overwrite the first's new entries. The PR description says this "ensures that checks... are additive rather than destructive," but that guarantee only holds when one process completes its rename before the other's inner read — a timing assumption that cannot be relied upon.

Concrete failure path:

  1. Process A reads disk → {checked: []}
  2. Process B reads disk → {checked: []} (before A writes)
  3. Process A renames → ["file_A"] persisted
  4. Process B renames → ["file_B"] persisted, "file_A" silently lost

The window is now the disk I/O span of lines 87–97 rather than the entire loadState→saveState call chain, which is a meaningful improvement, but the window is not zero. Truly eliminating it would require an OS-level exclusive lock (e.g. lockfile / proper-lockfile) around the read-modify-write, or a retry loop that re-reads after the rename and merges again if the file changed.

} catch (_) { /* ignore */ }
} catch (_) { /* ignore all errors to ensure hook never blocks */ }
}

function markChecked(key) {
Expand Down Expand Up @@ -262,4 +285,4 @@ function run(rawInput) {
return rawInput; // allow
}

module.exports = { run };
module.exports = { run };
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing newline at end of file

The diff shows \ No newline at end of file. POSIX requires text files to end with a newline, and many tools (diffing, cat, linters) behave unexpectedly without one.

Suggested change
module.exports = { run };
module.exports = { run };