fix: clean up upload directories and session state on TO2 failure

[sarmahaj]

• Track GUID-based upload directories created during fdo.upload in TO2 and clean them up if the session fails or the device disconnects, preventing orphaned partial uploads from accumulating on disk.
• Clean up related session state table rows (to2_sessions, to1_sessions, to0_sessions, etc.) in InvalidateToken, which previously left them orphaned on every failed session.

[gemini-code-assist]

Code Review

This pull request implements a cleanup mechanism for temporary upload directories and orphaned session states. It introduces tracking for GUID-based directories and ensures their removal upon session failure or invalidation. Additionally, the database logic is updated to prune related session tables when a token is invalidated. Feedback focuses on the performance impact of sweeping stale states on every cleanup, the omission of the DeviceInfo table from the cleanup list, and the need for database transactions to ensure atomicity during session invalidation.

[github-actions]

go-test-coverage report

Total test coverage: 62.9% (1771/2815)

Test coverage has changed in the current files, with 92 lines missing coverage.
  file:			uncovered:	current coverage:	base coverage:
  internal/db/state.go	 92		49.5% (90/182)		47.3% (80/169)

[sarmahaj]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to clean up GUID-based upload directories and orphaned session states. It updates the state machine to track created directories and ensures they are deleted if a session does not complete successfully. Furthermore, it enhances the database's token invalidation process by transactionally removing related session data across multiple tables. Feedback focuses on a critical race condition and performance issues in the state sweeping logic, as well as a potential logic error where the completion status fails to distinguish between successful execution and error-induced termination.

[gemini-code-assist]

This function introduces two significant issues:

1. Race Condition: It iterates over and modifies the s.states map without any synchronization. Since moduleStateMachines is used within concurrent HTTP handlers, this will lead to race conditions and potential panics. The states map should be protected by a sync.RWMutex (and methods should use pointer receivers).
2. Performance Bottleneck: It performs a full iteration of the map and a database query (SessionExists) for every entry, every time a session is cleaned up. This results in $O(N^2)$ complexity relative to the number of active sessions.

Consider moving the stale state sweeping to a background goroutine with a periodic ticker instead of triggering it on every session cleanup.

[gemini-code-assist]

The Completed flag is set to true whenever the iterator finishes, but it does not distinguish between a successful completion and an early exit due to an error (e.g., a database failure inside ownerModules). This results in errors being swallowed and may prevent proper cleanup of upload directories on failure. Consider capturing errors from the iterator and only marking the session as completed if it finished successfully.

[kgiusti]

Hi @sarmahaj - I'm playing with this fix and I've found a weird behavior. It may be due to a path not being covered by the cleanup path. Still investigating, but I think you may be able to reproduce this.

I wanted to see what happens if TO2 fails due to a problem uploading a file. So I modified the test/ci/test-fsim-upload.sh test to try to upload a file that doesn't exist. Here I added a file to upload that does not exist:

diff --git a/test/ci/test-fsim-upload.sh b/test/ci/test-fsim-upload.sh
index 3b2a6cb..a05755c 100755
--- a/test/ci/test-fsim-upload.sh
+++ b/test/ci/test-fsim-upload.sh
@@ -44,6 +44,7 @@ owner:
             - src: "${device_files[1]}"
               dst: "${owner_files[1]}"
             - src: "${device_files[2]}"
+            - src: "nopeIdonotexist"
 EOF
 }

Then ran the test: bash -c "source ./test/ci/test-fsim-upload.sh; run_test"

I'm seeing TO2 fail:

[14:08:07] INFO: Attempting TO1 protocol
[14:08:08] INFO: TO1 succeeded
  base URL: http://rendezvous:8041
[14:08:08] INFO: Attempting TO2 protocol
[14:08:13] ERROR: TO2 failed
  base URL: http://owner:8043
  error: error reading KV to send to owner: could not read service info key: error uploading "nopeIdonotexist": open /home/kgiusti/work/fdo/go-fdo-server/test/workdir/device-credentials/nopeIdonotexist: no such file or directory
[14:08:18] ERROR: TO2 failed
  base URL: http://127.0.0.1:8043
  error: error reading KV to send to owner: could not read service info key: error uploading "nopeIdonotexist": open /home/kgiusti/work/fdo/go-fdo-server/test/workdir/device-credentials/nopeIdonotexist: no such file or directory
[14:08:18] INFO: Applying default delay for last directive
  delay: 1m55.013682632s

as expected. This repeats... as expected.

What I found was that new GUID subdirectories are being created in the test upload directory go-fdo-server/test/workdir/fsim/upload/owner/ each time the cycle repeats, but they are never cleaned up. Each TO2 failure cycle adds a new GUID subdir, but never cleans up the previous one.

The code looks like it should be cleaning that up, but as best as I can tell the CleanupModule state machine callback is never being called by the go-fdo library.

I don't think this failure to call CleanupModule is related to this change at all. Probably an issue elsewhere. I'll poke some more... let you know what I see.

But related: maybe there's a good FSIM CI test we can add to test this. Maybe an FSIM configuration that uploads a file, then runs an "fdo.command" that fails the first time it runs TO2, then passes the next time TO2 runs (saves some state to the filesystem to track this?).

[kgiusti]

And here you go: fido-device-onboard/go-fdo#231

That's preventing the owner from invoking the cleanup module path. Here's a proposed fix:

fido-device-onboard/go-fdo#232

[sarmahaj]

@kgiusti Thanks for digging into this !
Your fix look good to me. fido-device-onboard/go-fdo#232

In your test scenario, sweepStaleStates didn't help because it only runs inside CleanupModules, which was never called for any session due to the go-fdo bug. I am looking into adding an alternative trigger for the sweep (e.g., on new session creation) so it can catch orphans even when CleanupModules is never invoked. (also until the fix lands on go-fdo)

Agree on adding a CI test for this/similar failure path.

[sarmahaj]

Note

The two new CI tests (test-upload-cleanup and test-upload-cleanup-missing-file) build both the client and server against fido-device-onboard/go-fdo#232 to validate the full cleanup flow end to end. Once that fix lands in go-fdo and we bump the dependency, these tests will build against the released version instead.

[kgiusti]

The tests you added look great - thanks!

One question and one potential issue:

Q: Are the state.go changes related to the cleanup? I'm having trouble understanding why those changes are part of this patch.

Issue: I haven't looked too closely at this code before but I think there's a potential race here over the moduleStateMachines struct. This is a single-instance that is shared across all active TO2 sessions. The http server spawns a separate goroutine for each client connection, which means they are all touching the same moduleStateMachines instance.

I think a lock needs to be added to this structure and taken when the states map and lastSweep flags are being referenced.

[kgiusti]

Ah - and if I paid attention to the gemini review comments I would've seen that it already raised the race issue... ouch.

[djach7]

Two relatively small changes, nothing major

[djach7]

Would it make sense to leave a comment here that this is a best effort cleanup? Given that if the cleanup fails and this error is passed the directory still remains.

I suppose the other way to do it would be to track it for retry or something like that, but I think a comment as a disclaimer suffices.

[sarmahaj]

Good point — updated the doc comment to clarify this is best-effort cleanup. If os.RemoveAll fails, the directory is left in place and the error is logged. There's no retry path since the entry is removed from the in-memory states map regardless of outcome, so we lose track of it. A startup sweep for orphaned GUID subdirs could address that, but seems better as a follow-up.

[djach7]

I think this might result in a similar race issue to the one gemini documented elsewhere. The mutex fix gemini suggested for the states race should be able to fix this one too.

[sarmahaj]

thanks for pointing that ,added a mutex lock now

[sarmahaj]

The tests you added look great - thanks!

One question and one potential issue:

Q: Are the state.go changes related to the cleanup? I'm having trouble understanding why those changes are part of this patch.

@kgiusti
The state.go changes handle the database side of the same orphan problem. Previously InvalidateToken only deleted the Session row, leaving child rows (TO2Session, KeyExchange, etc.) orphaned in every failed or timed-out session. The upload dir cleanup handles filesystem orphans, and the InvalidateToken change handles database orphans. Both are triggered by the same TO2 failure/disconnect path, so they made sense together. Happy to split into a separate commit if you'd prefer clearer separation.

Issue: I haven't looked too closely at this code before but I think there's a potential race here over the moduleStateMachines struct. This is a single-instance that is shared across all active TO2 sessions. The http server spawns a separate goroutine for each client connection, which means they are all touching the same moduleStateMachines instance.

I think a lock needs to be added to this structure and taken when the states map and lastSweep flags are being referenced.

you're right. The states map was already shared across goroutines, but this PR adds lastSweep and sweepStaleStates() which iterates the map and deletes stale entries, increasing the exposure. I'll add a sync.Mutex to the struct and lock in Module(), NextModule(), and CleanupModules() to address this.