fixup! make permissions multi-threaded

avivkeller · Aug 27, 2026 · Sep 8, 2026 · Sep 10, 2026 · Sep 8, 2026 · 7e08680379793b407cc778e163e2c07cf39110a5
commit 7e08680379793b407cc778e163e2c07cf39110a5
diff --git a/src/glob/glob_walker.cc b/src/glob/glob_walker.cc
diff --git a/src/glob/glob_walker.h b/src/glob/glob_walker.h
@@ -3,6 +3,7 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
+#include <cstdint>
 #include <limits>
 #include <memory>
 #include <string>
@@ -51,6 +52,31 @@ struct WalkEntry {
   int type = UV_DIRENT_UNKNOWN;
 };
 
+// One call a walk off the main thread needs made to the exclude callback,
+// which only that thread can do. A directory's questions are recorded in
+// the order the callback would see them, together with what decides
+// whether each is asked at all, so the main thread can put exactly the
+// calls the walk on that thread would have made.
+struct Question {
+  static constexpr size_t kNoEntry = SIZE_MAX;
+  // ExcludesEntry(first, parent, type) when set, else ExcludesPath(first)
+  bool entry;
+  std::string first;
+  std::string parent;
+  int type;
+  // The listing entry the question belongs to; kNoEntry for none
+  size_t entry_id;
+  // For an entry question: whether "not excluded" makes the walk descend
+  // into the entry, which raises the entry's path question. For a path
+  // question: whether the walk descends regardless of the entry's answers.
+  bool descends;
+};
+
+struct Answer {
+  bool entry;
+  bool excluded;
+};
+
 class WalkerImpl;
 
 class Walk {
@@ -67,6 +93,16 @@ class Walk {
                 bool on_main_thread,
                 std::vector<WalkEntry>* out);
 
+  // A slice that stopped because a directory needs the exclude callback:
+  // the questions for the main thread, then the answers to resume with.
+  bool HasQuestions() const;
+  const std::vector<Question>& questions() const;
+  void SetAnswers(std::vector<Answer> answers);
+
+  // Paths the permission model denied off the main thread since the last
+  // call, for publishing from that thread
+  void TakeDenied(std::vector<std::string>* out);
+
   // Stops background scanning and joins the walk's threads. Idempotent;
   // safe while a slice is in flight (it will wind down promptly). Called
   // when the walk finishes or is cancelled so threads never wait for

diff --git a/src/glob/node_glob.cc b/src/glob/node_glob.cc
@@ -434,8 +434,7 @@ GlobRequest::GlobRequest(Environment* env,
       ThreadPoolWork(env, "glob"),
       filter_(std::move(filter)),
       walk_(env, options, includes, excludes),
-      with_file_types_(options.with_file_types),
-      inline_only_(env->permission()->enabled() || filter_ != nullptr) {
+      with_file_types_(options.with_file_types) {
   MakeWeak();
 }
 
@@ -447,9 +446,52 @@ void GlobRequest::MemoryInfo(MemoryTracker* tracker) const {
 }
 
 void GlobRequest::DoThreadPoolWork() {
-  const size_t max_results =
-      drain_ ? std::numeric_limits<size_t>::max() : kBatchSize;
-  done_ = walk_.RunSlice(max_results, inline_only_, &batch_);
+  // A pull spans several slices when the exclude callback is consulted
+  // in between; the batch keeps filling across them (and a directory
+  // visited before a stop may already have filled it).
+  size_t max_results = std::numeric_limits<size_t>::max();
+  if (!drain_) {
+    max_results = batch_.size() < kBatchSize ? kBatchSize - batch_.size() : 0;
+  }
+  done_ = walk_.RunSlice(max_results, /*on_main_thread=*/false, &batch_);
+}
+
+void GlobRequest::PublishDenials() {
+  Environment* env = AsyncWrap::env();
+  std::vector<std::string> denied;
+  walk_.TakeDenied(&denied);
+  for (const std::string& path : denied) {
+    env->permission()->PublishDenied(
+        env, permission::PermissionScope::kFileSystemRead, path);
+  }
+}
+
+void GlobRequest::AnswerQuestions() {
+  std::vector<Answer> answers;
+  size_t entry = Question::kNoEntry;
+  bool descends = false;  // whether the walk descends into `entry`
+  for (const Question& question : walk_.questions()) {
+    if (question.entry_id != entry) {
+      entry = question.entry_id;
+      descends = false;
+    }
+    bool excluded;
+    if (question.entry) {
+      excluded = filter_->ExcludesEntry(
+          question.first, question.parent, question.type);
+      if (!excluded && question.descends) descends = true;
+    } else {
+      // A subpattern is only asked about when the walk descends into it.
+      if (entry != Question::kNoEntry && !question.descends && !descends) {
+        continue;
+      }
+      excluded = filter_->ExcludesPath(question.first);
+    }
+    // The callback threw: its exception is pending, the walk is over.
+    if (filter_->failed()) return;
+    answers.push_back({question.entry, excluded});
+  }
+  walk_.SetAnswers(std::move(answers));
 }
 
 // Builds [paths, types|undefined, done]
@@ -469,30 +511,49 @@ MaybeLocal<Value> GlobRequest::Settle() {
 
 void GlobRequest::AfterThreadPoolWork(int status) {
   in_flight_ = false;
+  Environment* env = AsyncWrap::env();
   // Environment teardown or a cancelled queue entry must not call back
   // into JavaScript.
-  if (status == UV_ECANCELED || !AsyncWrap::env()->can_call_into_js()) {
+  if (status == UV_ECANCELED || !env->can_call_into_js()) {
     resolver_.Reset();
     done_ = true;
     walk_.Stop();
     MakeWeak();
     return;
   }
   if (cancelled_) done_ = true;
-  // An exclude callback that threw leaves its exception pending
-  if (filter_ != nullptr && filter_->failed()) {
-    resolver_.Reset();
-    done_ = true;
-    walk_.Stop();
-    return;
-  }
-  // A finished walk keeps no threads waiting for garbage collection.
-  if (done_) walk_.Stop();
-  Environment* env = AsyncWrap::env();
+
   HandleScope scope(env->isolate());
   Local<Context> context = env->context();
   Context::Scope context_scope(context);
   InternalCallbackScope callback_scope(this);
+
+  PublishDenials();
+
+  if (!done_ && walk_.HasQuestions()) {
+    errors::TryCatchScope try_catch(env);
+    AnswerQuestions();
+    if (try_catch.HasCaught() || filter_->failed()) {
+      // The exclude callback threw: the pull rejects with its exception.
+      done_ = true;
+      walk_.Stop();
+      batch_.clear();
+      Local<Promise::Resolver> resolver = resolver_.Get(env->isolate());
+      resolver_.Reset();
+      MakeWeak();
+      // A terminating isolate (worker shutdown) has nothing to pass on.
+      if (try_catch.HasTerminated() || !try_catch.HasCaught()) return;
+      USE(resolver->Reject(context, try_catch.Exception()));
+      return;
+    }
+    // The walk resumes with the answers.
+    in_flight_ = true;
+    ScheduleWork();
+    return;
+  }
+
+  // A finished walk keeps no threads waiting for garbage collection.
+  if (done_) walk_.Stop();
   Local<Promise::Resolver> resolver = resolver_.Get(env->isolate());
   resolver_.Reset();
   // The request is idle again, so it may be collected with its handle.
@@ -542,11 +603,6 @@ void GlobRequest::Pull(const FunctionCallbackInfo<Value>& args, bool drain) {
 
   request->resolver_.Reset(isolate, resolver);
   request->in_flight_ = true;
-  if (request->inline_only_) {
-    request->DoThreadPoolWork();
-    request->AfterThreadPoolWork(0);
-    return;
-  }
   // Keep the request alive for as long as the worker holds a pointer to it.
   request->ClearWeak();
   request->ScheduleWork();

diff --git a/src/glob/node_glob.h b/src/glob/node_glob.h
@@ -124,14 +124,16 @@ class GlobRequest : public AsyncWrap, public ThreadPoolWork {
  private:
   static void Pull(const v8::FunctionCallbackInfo<v8::Value>& args, bool drain);
   v8::MaybeLocal<v8::Value> Settle();
+  // The main-thread half of a slice: publishes the permission denials the
+  // walk met and puts the questions of a directory waiting on the exclude
+  // callback to it, in the order the walk would have.
+  void PublishDenials();
+  void AnswerQuestions();
 
   // Declared before the walk, which points at it.
   std::unique_ptr<JsExcludeFilter> filter_;
   Walk walk_;
   const bool with_file_types_;
-  // Slices run on the main thread when the exclude callback or the
-  // permission model needs to be there; neither can change mid-request.
-  const bool inline_only_;
   // Set by all(): the pull consumes the rest of the walk in one batch.
   bool drain_ = false;
   bool done_ = false;

diff --git a/src/permission/fs_permission.cc b/src/permission/fs_permission.cc
@@ -120,6 +120,7 @@ namespace permission {
 void FSPermission::Apply(Environment* env,
                          std::span<const std::string> allow,
                          PermissionScope scope) {
+  RwLock::ScopedWriteLock lock(lock_);
   for (const std::string& res : allow) {
     if (res == "*") {
       if (scope == PermissionScope::kFileSystemRead) {
@@ -138,6 +139,7 @@ void FSPermission::Apply(Environment* env,
 void FSPermission::Drop(Environment* env,
                         PermissionScope scope,
                         std::string_view param) {
+  RwLock::ScopedWriteLock lock(lock_);
   if (param.empty()) {
     // Drop all access for this scope
     if (scope == PermissionScope::kFileSystemRead ||
@@ -232,6 +234,7 @@ void FSPermission::GrantAccess(PermissionScope perm, const std::string& res) {
 bool FSPermission::is_granted(Environment* env,
                               PermissionScope perm,
                               std::string_view param = "") const {
+  RwLock::ScopedReadLock lock(lock_);
   switch (perm) {
     case PermissionScope::kFileSystem:
       return allow_all_in_ && allow_all_out_;

diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h
@@ -5,6 +5,7 @@
 
 #include <memory>
 #include <vector>
+#include "node_mutex.h"
 #include "permission/permission_base.h"
 #include "util.h"
 
@@ -18,6 +19,8 @@ class FSPermission final : public PermissionBase {
   void Drop(Environment* env,
             PermissionScope scope,
             std::string_view param) override;
+  // Safe to call from any thread: Drop() rewrites the trees on the
+  // thread that owns `env`, which this waits out.
   bool is_granted(Environment* env,
                   PermissionScope perm,
                   std::string_view param) const override;
@@ -183,6 +186,8 @@ class FSPermission final : public PermissionBase {
   void GrantAccess(PermissionScope scope, const std::string& param);
   void RevokeAccess(PermissionScope scope, const std::string& param);
   void RebuildTree(PermissionScope scope);
+  // Guards everything below: Apply() and Drop() write, is_granted() reads.
+  RwLock lock_;
   // fs granted on startup
   RadixTree granted_in_fs_;
   RadixTree granted_out_fs_;

diff --git a/src/permission/permission.cc b/src/permission/permission.cc
@@ -311,39 +311,55 @@ void Permission::EnableWarningOnly() {
   }
 }
 
-bool Permission::is_scope_granted(Environment* env,
+bool Permission::is_granted_quiet(Environment* env,
                                   PermissionScope permission,
                                   std::string_view res) const {
+  if (!enabled_) [[likely]] {
+    return true;
+  }
   CHECK(permission != PermissionScope::kPermissionsRoot &&
         permission != PermissionScope::kPermissionsCount);
   auto& perm_node = nodes_[static_cast<size_t>(permission)];
-  bool result = false;
-  if (perm_node) {
-    result = perm_node->is_granted(env, permission, res);
-  }
-
-  if (!result && !publishing_) {
-    auto ch = GetOrCreateChannel(env, permission);
-    if (ch && ch->HasSubscribers()) {
-      publishing_ = true;
-      v8::Isolate* isolate = env->isolate();
-      v8::HandleScope handle_scope(isolate);
-      v8::Local<v8::Context> context = env->context();
-      v8::MaybeLocal<v8::Value> values[] = {
-          PermissionToString(env, permission),
-          ToV8Value(context, res),
-          Undefined(isolate),
-      };
-      ch->Publish(
-          env,
-          GetPermissionDiagnosticsTemplate(env)->NewInstance(context, values));
-      publishing_ = false;
-    }
-  }
+  return perm_node && perm_node->is_granted(env, permission, res);
+}
 
+bool Permission::is_scope_granted(Environment* env,
+                                  PermissionScope permission,
+                                  std::string_view res) const {
+  const bool result = is_granted_quiet(env, permission, res);
+  if (!result) Publish(env, permission, res, /*dropped=*/false);
   return result;
 }
 
+void Permission::PublishDenied(Environment* env,
+                               PermissionScope permission,
+                               std::string_view res) const {
+  Publish(env, permission, res, /*dropped=*/false);
+}
+
+void Permission::Publish(Environment* env,
+                         PermissionScope scope,
+                         std::string_view res,
+                         bool dropped) const {
+  // A subscriber's own checks must not publish recursively
+  if (publishing_) return;
+  auto ch = GetOrCreateChannel(env, scope);
+  if (!ch || !ch->HasSubscribers()) return;
+  publishing_ = true;
+  v8::Isolate* isolate = env->isolate();
+  v8::HandleScope handle_scope(isolate);
+  v8::Local<v8::Context> context = env->context();
+  v8::MaybeLocal<v8::Value> values[] = {
+      PermissionToString(env, scope),
+      ToV8Value(context, res),
+      dropped ? v8::True(isolate).As<v8::Value>()
+              : Undefined(isolate).As<v8::Value>(),
+  };
+  ch->Publish(
+      env, GetPermissionDiagnosticsTemplate(env)->NewInstance(context, values));
+  publishing_ = false;
+}
+
 BaseObjectPtr<diagnostics_channel::Channel> Permission::GetOrCreateChannel(
     Environment* env, PermissionScope scope) const {
   CHECK(scope != PermissionScope::kPermissionsRoot &&
@@ -380,25 +396,7 @@ void Permission::Drop(Environment* env,
   }
 
   // Publish to diagnostics channel so observers can track drops
-  if (!publishing_) {
-    auto ch = GetOrCreateChannel(env, scope);
-    if (ch && ch->HasSubscribers()) {
-      publishing_ = true;
-      v8::Isolate* isolate = env->isolate();
-      v8::HandleScope handle_scope(isolate);
-      v8::Local<v8::Context> context = env->context();
-
-      v8::MaybeLocal<v8::Value> values[] = {
-          PermissionToString(env, scope),
-          ToV8Value(context, param),
-          v8::True(isolate),
-      };
-      ch->Publish(
-          env,
-          GetPermissionDiagnosticsTemplate(env)->NewInstance(context, values));
-      publishing_ = false;
-    }
-  }
+  Publish(env, scope, param, /*dropped=*/true);
 }
 
 void Initialize(Local<Object> target,