Repository navigation
Conversation
After one clean full pass, a policy-versioned marker in the storage folder lets later starts check only the root folder, the database and its -wal/-shm files, and the models/ and logs/ subtrees. A wrong mode, an access error or disabled enforcement removes the marker, so the next start walks the whole tree again. Remove empty fence lock directories after each locked operation, use a fixed temp name instead of listing the fence folder, and sweep orphan lock directories and old temp files once during the full pass. Refs cortexkit#878
|
Waiting for This PR needs a linked issue with the |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!shouldEnforcePrivateStoragePermissions()) { | ||
| removeMarker(); | ||
| return { tightened, failures }; |
There was a problem hiding this comment.
Marker removal only runs when tightenStorageTreeSync is called. Pi shares its database handle across sessions and /reload, and cached openDatabaseAsync calls skip that pass. After a user disables enforcement and reloads, rememberGitIdentity can replace a file with mode 0644 while the old marker remains. A later start with enforcement enabled trusts the marker and skips project-identities, leaving that file readable by other users.
Clear markers when enforcement is disabled, including when the database is already open.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/shared/storage-file-mutex.ts">
<violation number="1" location="packages/plugin/src/shared/storage-file-mutex.ts:140">
P2: This cleanup can make a contender fail with `ENOENT` before it gets a ticket: three sequential releases can remove the directory between each `ensurePrivateStorageDirectorySync` and `wx` write, exhausting the two retries. Retry directory recreation until ticket creation succeeds (or coordinate cleanup with ticket creation) instead of using a fixed cap.</violation>
</file>
<file name="packages/plugin/src/hooks/magic-context/lkg-recovery-fence.ts">
<violation number="1" location="packages/plugin/src/hooks/magic-context/lkg-recovery-fence.ts:208">
P3: `locked` now removes only the fixed `${path}.tmp`, so a legacy random-name fence temp can remain indefinitely when startup is certified. Sweep legacy names on certified startup or invalidate the permission marker when this format changes.</violation>
</file>
<file name="packages/plugin/src/shared/storage-permissions.ts">
<violation number="1" location="packages/plugin/src/shared/storage-permissions.ts:262">
P2: The startup certificate trusts any regular `0600` marker without validating its version payload. A truncated or corrupted marker can therefore make the next startup skip the full permission walk; read the marker and require the exact expected contents before setting `certified`.</violation>
<violation number="2" location="packages/plugin/src/shared/storage-permissions.ts:262">
P1: Clear the certificate when enforcement is disabled, rather than only from this startup walk. A cached database open can bypass the walk, leaving a stale certificate that makes the next enforced start skip `project-identities` and retain a `0644` file.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| let certified = false; | ||
| try { | ||
| const stat = lstatSync(marker); | ||
| certified = stat.isFile() && (stat.mode & 0o7777) === 0o600; |
There was a problem hiding this comment.
P1: Clear the certificate when enforcement is disabled, rather than only from this startup walk. A cached database open can bypass the walk, leaving a stale certificate that makes the next enforced start skip project-identities and retain a 0644 file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/plugin/src/shared/storage-permissions.ts, line 262:
<comment>Clear the certificate when enforcement is disabled, rather than only from this startup walk. A cached database open can bypass the walk, leaving a stale certificate that makes the next enforced start skip `project-identities` and retain a `0644` file.</comment>
<file context>
@@ -219,21 +235,78 @@ export async function createStorageFileHandleAsync(
+ let certified = false;
+ try {
+ const stat = lstatSync(marker);
+ certified = stat.isFile() && (stat.mode & 0o7777) === 0o600;
+ if (!certified) removeMarker();
+ } catch (error) {
</file context>
| } finally { | ||
| rmSync(ownPath, { force: true }); | ||
| try { | ||
| rmdirSync(directory); |
There was a problem hiding this comment.
P2: This cleanup can make a contender fail with ENOENT before it gets a ticket: three sequential releases can remove the directory between each ensurePrivateStorageDirectorySync and wx write, exhausting the two retries. Retry directory recreation until ticket creation succeeds (or coordinate cleanup with ticket creation) instead of using a fixed cap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/plugin/src/shared/storage-file-mutex.ts, line 140:
<comment>This cleanup can make a contender fail with `ENOENT` before it gets a ticket: three sequential releases can remove the directory between each `ensurePrivateStorageDirectorySync` and `wx` write, exhausting the two retries. Retry directory recreation until ticket creation succeeds (or coordinate cleanup with ticket creation) instead of using a fixed cap.</comment>
<file context>
@@ -136,5 +136,10 @@ export function withStorageFileMutex<T>(directory: string, work: () => T, timeou
} finally {
rmSync(ownPath, { force: true });
+ try {
+ rmdirSync(directory);
+ } catch {
+ // Do not remove a live contender's ticket.
</file context>
| let certified = false; | ||
| try { | ||
| const stat = lstatSync(marker); | ||
| certified = stat.isFile() && (stat.mode & 0o7777) === 0o600; |
There was a problem hiding this comment.
P2: The startup certificate trusts any regular 0600 marker without validating its version payload. A truncated or corrupted marker can therefore make the next startup skip the full permission walk; read the marker and require the exact expected contents before setting certified.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/plugin/src/shared/storage-permissions.ts, line 262:
<comment>The startup certificate trusts any regular `0600` marker without validating its version payload. A truncated or corrupted marker can therefore make the next startup skip the full permission walk; read the marker and require the exact expected contents before setting `certified`.</comment>
<file context>
@@ -219,21 +235,78 @@ export async function createStorageFileHandleAsync(
+ let certified = false;
+ try {
+ const stat = lstatSync(marker);
+ certified = stat.isFile() && (stat.mode & 0o7777) === 0o600;
+ if (!certified) removeMarker();
+ } catch (error) {
</file context>
| rmSync(join(dirname(path), name), { force: true }); | ||
| } | ||
| // The mutex prevents concurrent use of the temp name. A folder listing is not necessary. | ||
| rmSync(`${path}.tmp`, { force: true }); |
There was a problem hiding this comment.
P3: locked now removes only the fixed ${path}.tmp, so a legacy random-name fence temp can remain indefinitely when startup is certified. Sweep legacy names on certified startup or invalidate the permission marker when this format changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/plugin/src/hooks/magic-context/lkg-recovery-fence.ts, line 208:
<comment>`locked` now removes only the fixed `${path}.tmp`, so a legacy random-name fence temp can remain indefinitely when startup is certified. Sweep legacy names on certified startup or invalidate the permission marker when this format changes.</comment>
<file context>
@@ -201,10 +204,8 @@ function locked<T>(sessionId: string, work: () => T): T {
- rmSync(join(dirname(path), name), { force: true });
- }
+ // The mutex prevents concurrent use of the temp name. A folder listing is not necessary.
+ rmSync(`${path}.tmp`, { force: true });
return work();
});
</file context>
Approved issue: #878
Harness coverage (required)
For each entry select: covered / not applicable — surface does not exist there / NOT covered.
shared/storage-permissions.ts,features/magic-context/storage-db.ts,shared/storage-file-mutex.ts,hooks/magic-context/lkg-recovery-fence.ts).What changes
Startup permission pass.
tightenStorageTreeSyncchecked every entry under the storage folder with synchronouslstatcalls, two times on each first database open in a process.STORAGE_PERMISSIONS_POLICY_VERSION), so a later change to the permission rules can force one more full pass.0600, the pass checks only the storage folder, the database file and its-waland-shmfiles, and themodels/andlogs/subtrees. Those two subtrees are written by code that does not set private modes, so they stay recursive.Fence folder.
withStorageFileMutexremoves its lock directory withrmdirafter the locked operation.rmdirfails while another process holds a ticket in the directory, and the error is ignored. Ticket creation already retries when the directory disappears betweenmkdirand the ticket write.<fence>.json.tmp, under the fence mutex.locked()removes only that name and no longer lists the folder on each operation.*.json.locksdirectories that have no fence file, and old temp files with random names from earlier versions. A temp file is removed only when it is older than 10 minutes (LEGACY_FENCE_TEMP_MIN_AGE_MS), because an older version in another process can still be writing it without the new mutex rules.Commits
fix(storage): bound certified startup permission checksfix(storage): keep fresh fence temp files during the full-pass sweepTests
storage-permissions-startup.test.ts: with a marker, a fixture with 2,000 fence files gave 2,004lstatcalls before the change (limit 8) and gives 7 after. A real SQLite reopen gave 4,008 before (limit 16) and gives 14 after. The first boot does a full pass and writes a private marker; a wrong mode removes the marker. When the marker check was removed, the startup test failed at 2,004 calls.storage-permissions-creators.test.ts: every creation path makes files0600and directories0700, including the database, WAL and SHM under umask 0. A creation that finds a wrong mode removes the marker.lkg-recovery-fence.test.ts,storage-file-mutex-review.test.ts: the.locksdirectory is gone after an operation and stays while a ticket is held; two operations in sequence on one fence work;locked()does not list the folder; the full pass removes empty orphan.locksdirectories and keeps the others. An old random-name temp file is removed, a fresh one stays, and unrelated names stay. The existing two-process publication race test passes.Results on this branch (Bun 1.4.0):
src/shared, storage database, fence and mutex tests): 1,055 pass, 0 fail.bun run typecheck, Biome on the changed files, andgit diff --check: clean.Summary by cubic
Skips the full storage permission walk on subsequent starts after one clean pass, cutting startup lstat calls without changing what gets enforced.
0600marker file makes later passes check only the storage root, the database and its-wal/-shmfiles, and themodels/andlogs/subtrees. Any failure, wrong mode, or disabled enforcement removes the marker and forces a full walk again..locksdirectory after each operation and uses a fixed temp name instead of listing the folder. The full pass now sweeps empty orphan.locksdirectories and legacy temp files older than 10 minutes.Written for commit 857c41f. Summary will update on new commits.
The PR appears safe to merge, with a non-blocking gap in marker removal when enforcement changes while a database handle is reused.
Findings
Summary
Adds a versioned marker so later database opens skip the full storage permission walk.
Acknowledged scope: iceteaSA explicitly chose the ten-minute age threshold for deleting legacy temporary files to preserve recent legacy writes.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Fresh database open] --> B{Enforcement enabled?} B -->|No| C[Remove marker] B -->|Yes| D{Private marker exists?} D -->|No| E[Walk full storage tree] E --> F[Write marker after success] D -->|Yes| G[Check root, database, models and logs] G --> H[Remove marker after repair or failure] I[Cached database open] --> J[Reuse handle without permission pass]Reviews (1) · Last reviewed commit: "fix(storage): keep fresh fence temp file..." · Reviewed by Greptile