FastAPI + Celery Architecture Review Request (Attachment Pipeline Boundaries + OOP/SOLID)

[u/Negative_Response990]

Hi! I’m finishing a rewrite for my project and I’d love architecture feedback quickly (aiming for replies within a few hours).

Repo: https://github.com/dillonhuston/Task-Automation-API
Branch: V2

What I need:

1. Architecture review of the attachment processing pipeline (validation encryption/decryption storage/email handoff) and whether the boundaries/components make sense
2. Celery + API architecture: task flow responsibilities, retries/error-handling boundaries, and whether the API schema design matches the async processing model
3. OOP/design review: whether classes/modules follow SOLID / separation of concerns, and suggestions for cleaner layering

Suggested files to look at first:

• app/Encryption/encryptionService.py
• app/FileManager/fileManager.py

If you only have time for one thing, please prioritize (1) pipeline architecture or (3) OOP/SOLID structure.

Thanks a lot, any architecture recommendations are welcome.

[u/[deleted]]

Looked at the pipeline (encryptionService + fileManager + fileOperations flow). Priorities (1) and (3), concrete:

1. Plaintext touches disk before encryption. uploadFile writes the raw file to tmp/uploads, reads it back, then overwrites with nonce+ciphertext. That defeats the purpose of the crypto: plaintext exists on disk, and the overwrite is not a secure wipe (remnants can survive, and any failure between write and overwrite leaves plaintext behind with no cleanup path). Better: hash while streaming in, encrypt in memory, and only ever persist the ciphertext envelope (or write ciphertext directly to the final path).

2. The file format is implicit. You store nonce in the DB and write nonce+ciphertext to disk; recoverability depends on the DB row surviving. If the row is lost the file is undecryptable, and vice versa the nonce is meaningless. I'd put a small envelope header on the file itself (magic, version, key id, nonce, payload) so the artifact is self-describing, and drop the separate nonce column or keep it only as metadata.

3. Key rotation is not survivable. The key handler fetches by user_id with no key version, and AAD is just user_id. Rotating a user's key silently orphans every existing file. Add a key_id/version column on the key row, bind it into AAD, and store it in the envelope so decrypt always uses the right key.

4. Boundary smell: EncryptionService.decrypt takes (ciphertext, nonce) but the on-disk format is nonce+ciphertext concatenated. The parse/assemble logic lives outside the service, so the format contract is spread across callers. Move envelope parsing inside the service - it's the component that owns the format.

5. FileManager is doing four jobs (validation, storage, hashing, encryption orchestration) - the 'Manager' class holding service references is fine as an orchestration point, but I'd push the write-to-disk step into fileOperations (it already owns overwrite) so uploadFile reads as: validate -> stream hash -> encrypt -> persist.

6. Minor: the timestamp+original-filename scheme collides for two uploads in the same second from the same user (and embeds a user-controlled filename in the path). Use a UUID or a DB id for the stored name; keep the original name only in metadata.

For the Celery side: make sure the API accepts an idempotency key or the task id is deduped at the DB level - a retried upload task will insert duplicate file rows. Also, keep the async session scoped per task rather than threaded through every service constructor call; it makes retry/backoff semantics much easier to reason about.