fix: link existing Spoolman spool when writing tag from picker (#130)#152
Conversation
When a spool is selected from the Spoolman picker and Write Tag is clicked, the JS calls POST /api/spoolman/pending-link with the selected spool ID before waiting for a tag. The firmware stores this pending link and on the next tag detection, PATCHes nfc_id on the existing spool instead of auto-creating a duplicate. Pending link expires after 2 minutes if no tag is detected. - SpoolmanManager: setPendingLink(), atomic pendingLinkSpoolId_ + expiry - WebServerManager: POST /api/spoolman/pending-link endpoint - SharedJS: track _selectedSpoolId in picker, call pending-link at write start
Store timestamp before spool ID in setPendingLink so any reader that sees a valid ID is guaranteed the timestamp is already set. Read timestamp before the exchange in syncSpool to match.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA pending-link mechanism is implemented to prevent spool duplication in Spoolman. When a spool is selected for writing, the firmware records it and sends a POST request marking it pending. Before normal spool sync, the firmware checks for valid pending links and patches the selected spool's NFC ID instead of auto-creating a duplicate. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SharedJS as SharedJS.h
participant WebServer as WebServerManager
participant SpoolMan as SpoolmanManager
participant SpoolmanAPI as Spoolman API
Client->>SharedJS: selectSpoolmanSpool(spoolId)
SharedJS->>SharedJS: Record _selectedSpoolId
Client->>SharedJS: sharedWriteFlow(config)
SharedJS->>WebServer: POST /api/spoolman/pending-link {spool_id}
WebServer->>SpoolMan: setPendingLink(spoolId)
SpoolMan->>SpoolMan: Store spool ID & timestamp atomically
WebServer-->>SharedJS: 200 OK {success: true}
SharedJS->>SharedJS: Proceed with tag write
Note over SpoolMan: Later, when tag detected...
SpoolMan->>SpoolMan: syncSpool() called
SpoolMan->>SpoolMan: Check pending link (atomic load)
alt Pending link valid
SpoolMan->>SpoolmanAPI: PATCH /api/v1/spool/{id} {nfc_id}
SpoolmanAPI-->>SpoolMan: 200 OK
SpoolMan->>SpoolMan: Update NFC ID cache
else Pending link expired
SpoolMan->>SpoolMan: Log and discard
end
SpoolMan->>SpoolMan: Continue normal sync logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SharedJS.h`:
- Around line 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.
In `@src/SpoolmanManager.cpp`:
- Around line 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.
In `@src/SpoolmanManager.h`:
- Around line 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.
In `@src/WebServerManager.cpp`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bf52bc0-e0ae-428d-958a-5dd4ded7342e
📒 Files selected for processing (5)
src/SharedJS.hsrc/SpoolmanManager.cppsrc/SpoolmanManager.hsrc/WebServerManager.cppsrc/WebServerManager.h
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| // 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| std::atomic<int32_t> pendingLinkSpoolId_{-1}; | ||
| std::atomic<uint32_t> pendingLinkSetAt_{0}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the SpoolmanManager.h file
find . -name "SpoolmanManager.h" -type fRepository: SpoolSense/spoolsense_scanner
Length of output: 96
🏁 Script executed:
# Examine the atomic declarations and setPendingLink method
grep -n "pendingLink" src/SpoolmanManager.h | head -30Repository: 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.hRepository: SpoolSense/spoolsense_scanner
Length of output: 595
🏁 Script executed:
# Find setPendingLink implementation
grep -n "void setPendingLink" src/SpoolmanManager.cpp -A 10Repository: 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 -20Repository: 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 -120Repository: 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 3Repository: 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 pendingRepository: SpoolSense/spoolsense_scanner
Length of output: 55
🏁 Script executed:
# Look for memory ordering semantics (memory_order_*)
grep -n "memory_order" src/SpoolmanManager.cppRepository: 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.
| 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}"); | ||
| } |
There was a problem hiding this comment.
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.
…pgrade PlatformIO auto-updated to espressif32@53.3.10 (Arduino ESP32 3.x / ESP-IDF 5.x) which has breaking API changes. Pin to last known-good 2.x-based platform version.
1 participant
Summary
POST /api/spoolman/pending-linkwith the selected spool ID before waiting for a tagsyncSpool()patchesnfc_idon the existing spool instead of auto-creating a duplicateChanges
SpoolmanManager:setPendingLink(int32_t), atomicpendingLinkSpoolId_+pendingLinkSetAt_, intercept block insyncSpool()WebServerManager:POST /api/spoolman/pending-linkendpointSharedJS: track_selectedSpoolIdin picker, call pending-link at write start for all four writer pagesTest plan
nfc_idset, no duplicate createdCloses #130
Summary by CodeRabbit