Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion platformio.ini
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
; https://docs.platformio.org/page/projectconf.html

[env]
platform = espressif32
platform = espressif32@6.10.0
framework = arduino
build_type = release
board_build.partitions = partitions.csv
Expand Down
13 changes: 13 additions & 0 deletions src/SharedJS.h
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ function prefillFromTag(fieldMap) {

var _spoolmanPickerSpools = [];
var _spoolmanPickerFieldMap = {};
var _selectedSpoolId = -1;

function renderSpoolmanPicker(containerId, fieldMap) {
var container = document.getElementById(containerId);
Expand Down Expand Up @@ -434,6 +435,7 @@ function filterSpoolmanPicker(spools, query, fieldMap) {
}

function selectSpoolmanSpool(spoolId) {
_selectedSpoolId = spoolId;
var spool = _spoolmanPickerSpools.find(function(s) { return s.id === spoolId; });
if (spool) fillFromSpoolman(spool, _spoolmanPickerFieldMap);
}
Expand Down Expand Up @@ -556,6 +558,17 @@ async function sharedWriteFlow(config) {
backBtn.classList.add('hidden');
anotherBtn.classList.add('hidden');

if (_selectedSpoolId > 0) {
try {
await api('/api/spoolman/pending-link', {
method: 'POST',
headers: {'Content-Type': 'application/json'},
body: JSON.stringify({spool_id: _selectedSpoolId})
});
} catch(e) {}
_selectedSpoolId = -1;
}
Comment on lines +561 to +570
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Abort the write when pending-link arming fails.

The empty catch here silently falls back to the old write path and still clears the selected spool ID, so a transient POST failure reintroduces the duplicate-spool behavior this PR is meant to remove. Surface the error and stop the write instead of continuing as if the link were armed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/SharedJS.h` around lines 561 - 570, The current empty catch around the
pending-link POST silently swallows failures and clears _selectedSpoolId,
allowing the old write path to proceed; change the error handling in the
try/catch that calls api('/api/spoolman/pending-link', ...) so that on any
exception you surface the error and abort the write (e.g. rethrow or return an
error state) and do not clear _selectedSpoolId when the POST fails. Locate the
try/catch that references _selectedSpoolId and the pending-link API call and
ensure failures stop further processing and preserve the selected spool ID until
a successful arm.


try {
var presentStatus = await waitForTag(8000);

Expand Down
34 changes: 34 additions & 0 deletions src/SpoolmanManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1249,6 +1249,14 @@ bool SpoolmanManager::lookupSpoolByUid(const char* uid, SpoolDetails& outDetails
return ok;
}

void SpoolmanManager::setPendingLink(int32_t spoolId) {
// Store timestamp before ID so that any reader seeing a valid ID is
// guaranteed the timestamp is already set (happens-before).
pendingLinkSetAt_.store(millis());
pendingLinkSpoolId_.store(spoolId);
Serial.printf("SpoolmanManager: Pending link set for spool %d\n", spoolId);
}

float SpoolmanManager::deductFromSpoolman(const char* uid, float grams) {
if (!isConfigured()) return 0.0f;
if (xSemaphoreTake(httpMutex_, HTTP_MUTEX_TIMEOUT) != pdTRUE) {
Expand Down Expand Up @@ -1322,6 +1330,32 @@ bool SpoolmanManager::syncSpool(const SpoolmanSyncRequest& req, int& resolvedSpo
resolvedSpoolmanId = -1;
bool success = false;

// If the writer pre-registered a spool to link, consume it and patch nfc_id before syncing.
// This prevents auto-sync from creating a duplicate when a pre-selected spool exists.
// Read timestamp before exchange: setPendingLink stores time before ID, so a valid ID
// guarantees the timestamp is already set (no race window on the age check).
uint32_t linkSetAt = pendingLinkSetAt_.load();
int32_t linkSpoolId = pendingLinkSpoolId_.exchange(-1);
if (linkSpoolId > 0) {
uint32_t age = millis() - linkSetAt;
if (age < PENDING_LINK_TIMEOUT_MS) {
char patchBody[64];
snprintf(patchBody, sizeof(patchBody), "{\"extra\":{\"nfc_id\":\"\\\"%s\\\"\"}}", req.spool_id);
char patchPath[48];
snprintf(patchPath, sizeof(patchPath), "/api/v1/spool/%d", linkSpoolId);
String patchResp;
int patchCode = httpPatch(patchPath, patchBody, patchResp);
if (patchCode == 200) {
storeCachedSpoolmanId(req.spool_id, linkSpoolId);
Serial.printf("SpoolmanManager: Linked nfc_id=%s to spool %d via pending link\n", req.spool_id, linkSpoolId);
} else {
Serial.printf("SpoolmanManager: Pending link PATCH failed (HTTP %d) for spool %d\n", patchCode, linkSpoolId);
}
} else {
Serial.printf("SpoolmanManager: Pending link for spool %d expired (%ums old), discarding\n", linkSpoolId, age);
}
}
Comment on lines +1333 to +1357
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Clear the previous owner of this NFC ID when applying the pending link.

This block patches req.spool_id onto linkSpoolId, but it never removes that same nfc_id from any older spool. The later global UID lookup still treats duplicate matches as valid and picks one by ID, so a stale blank/old spool can continue to win after cache loss or reboot. Reuse the existing "link new, then clear old" behavior here, or explicitly clear/archive the previous holder around this PATCH.

🧰 Tools
🪛 Clang (14.0.6)

[note] 1339-1339: +1, including nesting penalty of 0, nesting level increased to 1

(clang)


[note] 1341-1341: +2, including nesting penalty of 1, nesting level increased to 2

(clang)


[note] 1348-1348: +3, including nesting penalty of 2, nesting level increased to 3

(clang)


[note] 1351-1351: +1, nesting level increased to 3

(clang)


[note] 1354-1354: +1, nesting level increased to 2

(clang)


[warning] 1337-1337: variable 'linkSetAt' is not initialized

(cppcoreguidelines-init-variables)


[warning] 1338-1338: variable 'linkSpoolId' is not initialized

(cppcoreguidelines-init-variables)


[warning] 1342-1342: do not declare C-style arrays, use std::array<> instead

(modernize-avoid-c-arrays)


[warning] 1343-1343: escaped string literal can be written as a raw string literal

(modernize-raw-string-literal)


[warning] 1344-1344: do not declare C-style arrays, use std::array<> instead

(modernize-avoid-c-arrays)


[warning] 1346-1346: variable 'patchResp' is not initialized

(cppcoreguidelines-init-variables)


[warning] 1347-1347: variable 'patchCode' is not initialized

(cppcoreguidelines-init-variables)


[warning] 1348-1348: if with identical then and else branches

(bugprone-branch-clone)


[note] 1351-1351: else branch starts here

(clang)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/SpoolmanManager.cpp` around lines 1333 - 1357, When applying the pending
link (using pendingLinkSpoolId_, linkSpoolId, req.spool_id and httpPatch()),
first clear any existing owner that already has that nfc_id: perform a lookup
for a spool with nfc_id == req.spool_id (or call the existing helper that finds
a spool by nfc_id) and if it returns an ID != linkSpoolId, PATCH that older
spool to remove/clear its nfc_id before PATCHing linkSpoolId; then proceed with
the current PATCH to set req.spool_id on linkSpoolId and call
storeCachedSpoolmanId(req.spool_id, linkSpoolId) as before so the old holder
cannot win after cache loss or reboot.


// Prefer a known-good ID for this spool UID over potentially stale tag data.
int32_t preferredSpoolmanId = req.spoolman_id;
int32_t cachedSpoolmanId = lookupCachedSpoolmanId(req.spool_id);
Expand Down
8 changes: 8 additions & 0 deletions src/SpoolmanManager.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef SPOOLMAN_MANAGER_H
#define SPOOLMAN_MANAGER_H

#include <atomic>
#include <cstdint>
#include <freertos/FreeRTOS.h>
#include <freertos/queue.h>
Expand Down Expand Up @@ -53,6 +54,9 @@ class SpoolmanManager {
bool isConfigured() const;
bool getSpoolDetails(int32_t spoolmanId, SpoolDetails& outDetails);
void invalidateCachedSpoolmanId(const char* spoolId);
// Pre-emptively link the next detected tag to an existing spool instead of auto-creating.
// Set before write flow starts; consumed on first tag sync within PENDING_LINK_TIMEOUT_MS.
void setPendingLink(int32_t spoolId);

// Deduct weight directly in Spoolman for non-writable tags.
// Returns grams deducted, or 0 on failure (caller should retry later).
Expand Down Expand Up @@ -99,6 +103,10 @@ class SpoolmanManager {
static constexpr UBaseType_t TASK_PRIORITY = 1;
static constexpr TickType_t HTTP_MUTEX_TIMEOUT = pdMS_TO_TICKS(10000);
static constexpr uint32_t SYNC_CACHE_TTL_MS = 2 * 60 * 60 * 1000; // 2 hours
static constexpr uint32_t PENDING_LINK_TIMEOUT_MS = 120000; // 2 minutes

std::atomic<int32_t> pendingLinkSpoolId_{-1};
std::atomic<uint32_t> pendingLinkSetAt_{0};
Comment on lines +108 to +109
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the SpoolmanManager.h file
find . -name "SpoolmanManager.h" -type f

Repository: SpoolSense/spoolsense_scanner

Length of output: 96


🏁 Script executed:

# Examine the atomic declarations and setPendingLink method
grep -n "pendingLink" src/SpoolmanManager.h | head -30

Repository: SpoolSense/spoolsense_scanner

Length of output: 178


🏁 Script executed:

# Look at the complete declaration context around lines 106-109
sed -n '100,115p' src/SpoolmanManager.h

Repository: SpoolSense/spoolsense_scanner

Length of output: 595


🏁 Script executed:

# Find setPendingLink implementation
grep -n "void setPendingLink" src/SpoolmanManager.cpp -A 10

Repository: SpoolSense/spoolsense_scanner

Length of output: 55


🏁 Script executed:

# Find syncSpool method and examine how it reads the pending link state
grep -n "syncSpool" src/SpoolmanManager.cpp -A 50 | grep -E "(pendingLink|load|exchange)" | head -20

Repository: SpoolSense/spoolsense_scanner

Length of output: 922


🏁 Script executed:

# Get the full syncSpool implementation to see how pendingLink state is used
grep -n "void SpoolmanManager::syncSpool" src/SpoolmanManager.cpp -A 100 | head -120

Repository: SpoolSense/spoolsense_scanner

Length of output: 55


🏁 Script executed:

# Search for mutexes or locks around pendingLink operations
grep -n "pendingLink" src/SpoolmanManager.cpp -B 3 -A 3

Repository: SpoolSense/spoolsense_scanner

Length of output: 1003


🏁 Script executed:

# Check if there are any guards (mutexes, spinlocks) protecting pendingLink access
grep -n "mutex\|lock\|Guard" src/SpoolmanManager.h | grep -i pending

Repository: SpoolSense/spoolsense_scanner

Length of output: 55


🏁 Script executed:

# Look for memory ordering semantics (memory_order_*)
grep -n "memory_order" src/SpoolmanManager.cpp

Repository: SpoolSense/spoolsense_scanner

Length of output: 55


Address the race condition in pending link state updates.

pendingLinkSpoolId_ and pendingLinkSetAt_ are updated and read as independent atomics (lines 1255–1256 and 1337–1338). The comment claims that storing time before ID prevents races, but a reader can load the old timestamp before the new one is published, then load the new ID after it's published. This creates a window where syncSpool() observes a stale timestamp paired with a fresh spool ID, causing valid pending links to be incorrectly expired.

Combine both values into a single atomic word (e.g., a packed struct or 64-bit integer) or protect both with a mutex so readers always observe a consistent snapshot.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/SpoolmanManager.h` around lines 108 - 109, The current race between
pendingLinkSpoolId_ and pendingLinkSetAt_ can lead syncSpool() to see a stale
timestamp with a new spool ID; fix by making the two fields atomic together:
replace the separate std::atomic<int32_t> pendingLinkSpoolId_ and
std::atomic<uint32_t> pendingLinkSetAt_ with a single atomic 64-bit word (or
packed struct) that encodes both spoolId and setAt, or alternatively guard both
with a mutex so updates/readers use the same lock; update all accesses in code
(places that read/write pendingLinkSpoolId_, pendingLinkSetAt_, and the
syncSpool() checks) to use the new combined atomic or the mutex-protected
getter/setter to ensure readers always observe a consistent snapshot.

};

#endif // SPOOLMAN_MANAGER_H
23 changes: 22 additions & 1 deletion src/WebServerManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ bool WebServerManager::begin(bool apMode, uint16_t port) {
_server.on("/api/write-openspool", HTTP_POST, [this]() { handleApiWriteOpenSpool(); });
_server.on("/api/register-uid", HTTP_POST, [this]() { handleApiRegisterUid(); });
_server.on("/api/spoolman/spools", HTTP_GET, [this]() { handleApiSpoolmanSpools(); });
_server.on("/api/spoolman/link", HTTP_POST, [this]() { handleApiSpoolmanLink(); });
_server.on("/api/spoolman/link", HTTP_POST, [this]() { handleApiSpoolmanLink(); });
_server.on("/api/spoolman/pending-link", HTTP_POST, [this]() { handleApiSpoolmanPendingLink(); });
_server.on("/api/spoolman/find-vendor", HTTP_GET, [this]() { handleApiSpoolmanFindVendor(); });
_server.on("/api/spoolman/find-filament", HTTP_GET, [this]() { handleApiSpoolmanFindFilament(); });
_server.on("/api/spoolman/save-enrichment", HTTP_POST, [this]() { handleApiSpoolmanSaveEnrichment(); });
Expand Down Expand Up @@ -614,6 +615,26 @@ void WebServerManager::handleApiSpoolmanLink() {
_server.send(200, "application/json", "{\"success\":true}");
}

void WebServerManager::handleApiSpoolmanPendingLink() {
_server.sendHeader("Access-Control-Allow-Origin", "*");

StaticJsonDocument<128> doc;
if (deserializeJson(doc, _server.arg("plain"))) {
sendError(400, "Invalid JSON");
return;
}

int spoolId = doc["spool_id"] | -1;
if (spoolId <= 0) {
sendError(400, "spool_id required");
return;
}

SpoolmanManager::getInstance().setPendingLink(spoolId);
Serial.printf("WebServerManager: Pending link set for spool %d\n", spoolId);
_server.send(200, "application/json", "{\"success\":true}");
}
Comment on lines +618 to +636
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Add a way to cancel a pending link.

This endpoint only arms global pending-link state. If the client later times out, the user backs out, or the write fails, there is no way to disarm it, so the next unrelated sync inside the 2-minute window can overwrite that spool's nfc_id. Please add a clear/delete variant and have the writer call it on abort paths.

🧰 Tools
🪛 Clang (14.0.6)

[warning] 621-621: variable 'doc' is not initialized

(cppcoreguidelines-init-variables)


[warning] 627-627: variable 'spoolId' is not initialized

(cppcoreguidelines-init-variables)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/WebServerManager.cpp` around lines 618 - 636, The current
handleApiSpoolmanPendingLink only sets global pending link and lacks a way to
clear it; add a complementary endpoint (e.g., handleApiSpoolmanClearPendingLink
or extend handleApiSpoolmanPendingLink to accept an "action":"clear" parameter)
that calls a new SpoolmanManager::clearPendingLink() (or
SpoolmanManager::setPendingLink(-1)) to disarm the pending state; validate JSON
similarly, require no or a spool_id for safety, log the clear action
(Serial.printf) and return {"success":true}; update the writer/abort paths to
call this clear endpoint whenever the write is aborted or times out.


void WebServerManager::handleApiDiagnostics() {
_server.sendHeader("Access-Control-Allow-Origin", "*");

Expand Down
1 change: 1 addition & 0 deletions src/WebServerManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class WebServerManager {
void handleApiRegisterUid();
void handleApiSpoolmanSpools();
void handleApiSpoolmanLink();
void handleApiSpoolmanPendingLink();
void handleApiSpoolmanFindVendor();
void handleApiSpoolmanFindFilament();
void handleApiSpoolmanSaveEnrichment();
Expand Down