Reduce room board recovery stack use under sustained reads

examples/simple_room_server/MyMesh.cpp

@@ -46,6 +46,7 @@
 #include <WiFi.h>
 #include <Preferences.h>
 #include <ArduinoJson.h>
+#include <memory>
 #include <helpers/RoomWebJson.h>
 #include <helpers/RoomMailWeb.h>
 #endif
@@ -579,7 +580,11 @@ void MyMesh::processRoomWebBoard(const char* operation, JsonObjectConst input,
     json.reset();
     json.raw("{\"error\":"); json.string(message); json.raw("}"); json.finish();
   };
-  mesh::RoomBoardIndex board;
+  // Metadata remains live through filesystem calls. Keep it off the 8 KiB
+  // loop task stack and fail before storage access if scratch allocation fails.
+  std::unique_ptr<mesh::RoomBoardIndex> scratch(new (std::nothrow) mesh::RoomBoardIndex);
+  if (!scratch) { error("insufficient memory; information board retained"); return; }
+  mesh::RoomBoardIndex& board = *scratch;
   if (!mesh::loadRoomBoard(_fs, board)) { error("information board storage unavailable"); return; }
   if (strcmp(operation, "board.index") == 0) {
     if (!input["revision"].is<uint32_t>() || !input["cursor"].is<uint8_t>()) { error("invalid board cursor"); return; }
@@ -602,25 +607,49 @@ void MyMesh::processRoomWebBoard(const char* operation, JsonObjectConst input,
     json.raw("],\"next\":"); json.number(next < board.count ? next : 255);
     json.raw("}"); json.finish(); return;
   }
+  if (strcmp(operation, "board.read") == 0) {
+    processRoomWebBoardRead(input, board, json); return;
+  }
+  processRoomWebBoardWrite(operation, input, board, json);
+}
+
+void MyMesh::processRoomWebBoardRead(JsonObjectConst input, const mesh::RoomBoardIndex& board,
+                                     mesh::RoomJsonWriter& json) {
+  auto error = [&json](const char* message) {
+    json.reset();
+    json.raw("{\"error\":"); json.string(message); json.raw("}"); json.finish();
+  };
+  if (!input["id"].is<uint8_t>() || !input["version"].is<uint32_t>()) { error("invalid article identity/version"); return; }
+  const uint8_t id = input["id"].as<uint8_t>();
+  const uint32_t version = input["version"].as<uint32_t>();
+  const mesh::RoomBoardArticle* article = nullptr;
+  for (uint8_t i = 0; i < board.count; ++i) if (board.articles[i].id == id) article = &board.articles[i];
+  if (!input["offset"].is<uint16_t>()) { error("invalid article offset"); return; }
+  const uint16_t offset = input["offset"].as<uint16_t>();
+  uint8_t chunk[mesh::ROOM_BOARD_READ_MAX_CHUNK];
+  size_t length = 0;
+  const auto result = mesh::readRoomBoardArticle(_fs, id, version, offset, chunk, sizeof(chunk), length);
+  if (result != mesh::RoomBoardResult::Success || !article) { error("article changed or unavailable; reload index"); return; }
+  char encoded[4 * ((sizeof(chunk) + 2) / 3) + 1]; mesh::roomEncodeBase64(chunk, length, encoded);
+  json.raw("{\"version\":"); json.number(version);
+  json.raw(",\"offset\":"); json.number(offset);
+  json.raw(",\"next\":"); json.number(uint32_t(offset) + length);
+  json.raw(",\"total\":"); json.number(article->body_length);
+  json.raw(",\"data64\":"); json.string(encoded); json.raw("}"); json.finish();
+}
+
+void MyMesh::processRoomWebBoardWrite(const char* operation, JsonObjectConst input,
+                                      const mesh::RoomBoardIndex& board,
+                                      mesh::RoomJsonWriter& json) {
+  auto error = [&json](const char* message) {
+    json.reset();
+    json.raw("{\"error\":"); json.string(message); json.raw("}"); json.finish();
+  };
   if (!input["id"].is<uint8_t>() || !input["version"].is<uint32_t>()) { error("invalid article identity/version"); return; }
   const uint8_t id = input["id"].as<uint8_t>();
   const uint32_t version = input["version"].as<uint32_t>();
   const mesh::RoomBoardArticle* article = nullptr;
   for (uint8_t i = 0; i < board.count; ++i) if (board.articles[i].id == id) article = &board.articles[i];
-  if (strcmp(operation, "board.read") == 0) {
-    if (!input["offset"].is<uint16_t>()) { error("invalid article offset"); return; }
-    const uint16_t offset = input["offset"].as<uint16_t>();
-    uint8_t chunk[mesh::ROOM_BOARD_READ_MAX_CHUNK];
-    size_t length = 0;
-    const auto result = mesh::readRoomBoardArticle(_fs, id, version, offset, chunk, sizeof(chunk), length);
-    if (result != mesh::RoomBoardResult::Success || !article) { error("article changed or unavailable; reload index"); return; }
-    char encoded[4 * ((sizeof(chunk) + 2) / 3) + 1]; mesh::roomEncodeBase64(chunk, length, encoded);
-    json.raw("{\"version\":"); json.number(version);
-    json.raw(",\"offset\":"); json.number(offset);
-    json.raw(",\"next\":"); json.number(uint32_t(offset) + length);
-    json.raw(",\"total\":"); json.number(article->body_length);
-    json.raw(",\"data64\":"); json.string(encoded); json.raw("}"); json.finish(); return;
-  }
   mesh::RoomBoardResult result;
   if (strcmp(operation, "board.delete") == 0) {
     if (version == 0) { error("deletion requires the current article version"); return; }

examples/simple_room_server/MyMesh.h

@@ -367,6 +367,11 @@ class MyMesh : public mesh::Mesh, public CommonCLICallbacks,
                           mesh::RoomJsonWriter& json) __attribute__((noinline));
   void processRoomWebBoard(const char* operation, JsonObjectConst input,
                           mesh::RoomJsonWriter& json) __attribute__((noinline));
+  void processRoomWebBoardRead(JsonObjectConst input, const mesh::RoomBoardIndex& board,
+                              mesh::RoomJsonWriter& json) __attribute__((noinline));
+  void processRoomWebBoardWrite(const char* operation, JsonObjectConst input,
+                               const mesh::RoomBoardIndex& board,
+                               mesh::RoomJsonWriter& json) __attribute__((noinline));
   bool _unconfigured_setup_espnow_suspended = false;
   void suspendUnconfiguredSetupBridges();
   bool startWebConfigImpl(bool force_ap, char* reply, bool automatic_setup);

src/helpers/RoomBoardStore.h

@@ -42,6 +42,13 @@ static constexpr size_t STREAM_CHUNK = 64;
 static const uint8_t INDEX_MAGIC[4] = {'R', 'B', 'I', 1};
 static const uint8_t ARTICLE_MAGIC[4] = {'R', 'B', 'A', 1};
 
+// Every field is an integer or character array. Clear the destination itself:
+// assigning a value-initialized index can put a second whole index on the
+// embedded caller's stack while filesystem recovery is still active.
+inline void clearIndex(RoomBoardIndex& index) {
+  memset(static_cast<void*>(&index), 0, sizeof(index));
+}
+
 inline uint32_t crc(uint32_t value, const uint8_t* data, size_t length) {
   while (length-- != 0) {
     value ^= *data++;
@@ -206,7 +213,7 @@ bool readIndex(Filesystem* fs, const char* path, RoomBoardIndex& staged,
     for (size_t i = 9; i < sizeof(header); ++i) valid = valid && header[i] == 0;
   }
   uint32_t checksum = valid ? crc(UINT32_C(0xffffffff), header, sizeof(header)) : 0;
-  staged = RoomBoardIndex{};
+  clearIndex(staged);
   if (valid) { staged.revision = get32(header + 4); staged.count = header[8]; }
   for (uint8_t i = 0; valid && i < staged.count; ++i) {
     uint8_t bytes[ENTRY_SIZE];
@@ -252,7 +259,7 @@ bool recover(Filesystem* fs, RoomBoardIndex& staged, bool require_cleanup) {
   if (present) {
     if (!readIndex(fs, ROOM_BOARD_BACKUP_PATH, staged)
         || !fs->rename(ROOM_BOARD_BACKUP_PATH, ROOM_BOARD_PRIMARY_PATH)) return false;
-  } else staged = RoomBoardIndex{};
+  } else clearIndex(staged);
   const bool temp_removed = removeArtifact(fs, ROOM_BOARD_TEMP_PATH);
   return !require_cleanup || temp_removed;
 }
@@ -295,10 +302,14 @@ RoomBoardResult commitIndex(Filesystem* fs, const RoomBoardIndex& next) {
 
 template <typename Filesystem>
 bool loadRoomBoard(Filesystem* fs, RoomBoardIndex& index) {
-  RoomBoardIndex staged;
-  index = RoomBoardIndex{};
-  if (!room_board_detail::recover(fs, staged, false)) return false;
-  index = staged; return true;
+  room_board_detail::clearIndex(index);
+  if (!room_board_detail::recover(fs, index, false)) {
+    // A corrupt body or a late I/O failure may follow valid metadata. Never
+    // return that partially populated index to a caller on failure.
+    room_board_detail::clearIndex(index);
+    return false;
+  }
+  return true;
 }
 
 // Main-loop single writer. The reader is called as reader(offset, buffer,

test/fixtures/room_web_service/service.cpp

@@ -1,6 +1,7 @@
 #include "filesystem.h"
 #include <cstdlib>
 #include <new>
+#include <memory>
 #include <array>
 #include <ArduinoJson.h>
 static bool deny_json_allocation = false;
@@ -24,6 +25,11 @@ static void* trackedRealloc(void* ptr, size_t size) {
 #undef free
 #undef realloc
 #include <helpers/RoomBoardStore.h>
+static bool deny_board_scratch_allocation = false;
+void* operator new(size_t size, const std::nothrow_t&) noexcept {
+  if (deny_board_scratch_allocation) return nullptr;
+  try { return ::operator new(size); } catch (const std::bad_alloc&) { return nullptr; }
+}
 #include <helpers/RoomMailWeb.h>
 #include <helpers/RoomHistoryStore.h>
 #include <helpers/RoomAccessPolicy.h>
@@ -172,6 +178,10 @@ struct MyMesh {
                          mesh::RoomJsonWriter&) __attribute__((noinline));
   void processRoomWebAdmin(const char*, JsonObjectConst, mesh::RoomJsonWriter&) __attribute__((noinline));
   void processRoomWebBoard(const char*, JsonObjectConst, mesh::RoomJsonWriter&) __attribute__((noinline));
+  void processRoomWebBoardRead(JsonObjectConst, const mesh::RoomBoardIndex&,
+                               mesh::RoomJsonWriter&) __attribute__((noinline));
+  void processRoomWebBoardWrite(const char*, JsonObjectConst, const mesh::RoomBoardIndex&,
+                                mesh::RoomJsonWriter&) __attribute__((noinline));
   void processRoomRequest(const uint8_t*, uint32_t, char*, char*, size_t);
   void serviceRoomQuotas();
   bool storePost(const mesh::Identity&, const char*);
@@ -1032,6 +1042,25 @@ int main() {
     assert(result(m, saveRequest(1, 0, "Reused", "new"), 36)["ok"] == true);
     error(m, request("board.read", "", ",\"id\":1,\"version\":2,\"offset\":0"), 37, "article changed"); ++scenarios;
   }
+  {
+    MyMesh m; enable(m);
+    assert(result(m, saveRequest(1, 0, "Retained", std::string(2048, 's')), 1)["ok"] == true);
+    std::map<std::string, std::vector<uint8_t>> before;
+    for (const auto& file : m.storage.files) before[file.first] = *file.second;
+    m.storage.trace.clear();
+    deny_board_scratch_allocation = true;
+    error(m, request("board.index", "", ",\"revision\":0,\"cursor\":0"), 2, "insufficient memory");
+    error(m, request("board.read", "", ",\"id\":1,\"version\":1,\"offset\":0"), 3, "insufficient memory");
+    error(m, saveRequest(1, 1, "Changed", "new"), 4, "insufficient memory");
+    error(m, request("board.delete", "admin-secret", ",\"boot\":71,\"id\":1,\"version\":1"), 5, "insufficient memory");
+    deny_board_scratch_allocation = false;
+    assert(m.storage.trace.empty() && m.storage.files.size() == before.size());
+    for (const auto& file : m.storage.files) assert(*file.second == before.at(file.first));
+    const auto page = result(m, request("board.index", "", ",\"revision\":0,\"cursor\":0"), 6);
+    assert(page["revision"] == 1 && page["articles"][0]["length"] == 2048);
+    assert(result(m, request("board.read", "", ",\"id\":1,\"version\":1,\"offset\":0"), 7)["total"] == 2048);
+    ++scenarios;
+  }
   {
     MyMesh m; enable(m); uint32_t sequence = 1;
     for (const std::string& input : {std::string("{"), std::string("[]"), std::string("null"), std::string("{\"op\":{\"a\":{\"b\":{\"c\":{\"d\":1}}}}}"),

test/test_room_board_store.py

@@ -95,7 +95,15 @@ static void checksum(std::vector<uint8_t>& image) {
 static void preserveUnavailable(MemoryFS& fs) {
   const auto before = snapshot(fs); const auto directories = fs.directories;
   Index index; index.count = 7; index.revision = 99;
+  for (auto& article : index.articles) {
+    article.id = 7; article.bank = 1; article.body_length = 2000; article.version = 98;
+    memset(article.title, 'x', sizeof(article.title));
+  }
   assert(!load(fs, index) && index.count == 0 && index.revision == 0);
+  for (const auto& article : index.articles) {
+    assert(article.id == 0 && article.bank == 0 && article.body_length == 0 && article.version == 0);
+    for (char value : article.title) assert(value == 0);
+  }
   assert(save(fs, 1, "Changed", NEW_BODY) == Result::Unavailable);
   assert(erase(fs, 1) == Result::Unavailable);
   uint8_t out[128]; size_t count = 99;

test/test_room_web_service.py

@@ -31,6 +31,7 @@ class RoomWebServiceTest(unittest.TestCase):
             "bool MyMesh::handleRoomWebCommand(", "static const char* roomWebString(",
             "bool MyMesh::processRoomWebMail(", "void MyMesh::processRoomWebAdmin(",
             "void MyMesh::processRoomWebBoard(",
+            "void MyMesh::processRoomWebBoardRead(", "void MyMesh::processRoomWebBoardWrite(",
             "void MyMesh::processRoomRequest(", "void MyMesh::serviceRoomQuotas(",
             "bool MyMesh::storePost(", "bool MyMesh::snapshotRoomHistory(",
             "bool MyMesh::executeClientPathCommand(", "bool MyMesh::setRoomClientPath(",
@@ -111,7 +112,8 @@ class RoomWebServiceTest(unittest.TestCase):
             usage = "\n".join(path.read_text() for path in work.glob("*.su"))
             frames = {}
             for method, budget in (("processRoomRequest", 1024), ("processRoomWebAdmin", 1536),
-                                   ("processRoomWebBoard", 2048), ("processRoomWebMail", 3072)):
+                                   ("processRoomWebBoard", 384), ("processRoomWebBoardRead", 1152),
+                                   ("processRoomWebBoardWrite", 1536), ("processRoomWebMail", 3072)):
                 sizes = re.findall(r"(?:void|bool) MyMesh::" + method
                                    + r"\([^\t\n]*\)\s*\t(\d+)\t[^\n]*", usage)
                 self.assertEqual(len(sizes), 1, "missing or ambiguous stack frame: " + method)