Skip to content

Update request_logger.py#10009

Closed
deshudiosh wants to merge 1 commit intoComfy-Org:masterfrom
deshudiosh:patch-1
Closed

Update request_logger.py#10009
deshudiosh wants to merge 1 commit intoComfy-Org:masterfrom
deshudiosh:patch-1

Conversation

@deshudiosh
Copy link
Copy Markdown

Fix for "Error writing API log to: ......"

Saving api logs failed when Seedream4 official template was used on windows machine, due to the forbidden "=?:" characters in the filename, as well as the filename being too long.

Regex keeps only usable characters, and 220 arbitrary max filename length is added.

api log fails

Saving api logs failed when Seedream4 official template was used on windows machine, due to the forbidden "=?:" characters in the filename, as well as the filename being too long.

Regex keeps only usable characters, and 220 arbitrary max filename length is added.
@Kosinkadink Kosinkadink added the Good PR This PR looks good to go, it needs comfy's final review. label Sep 26, 2025
@bigcat88
Copy link
Copy Markdown
Contributor

@Kosinkadink initial problem is in that the logger attach all headers to the path: Expires=1759228587&GoogleAccessId=35714895...

Maybe better to completely rewrite that to fix the root issue?

log_dir = get_log_directory()
timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S_%f")
filename = f"{timestamp}_{operation_id.replace('/', '_').replace(':', '_')}.log"
safe_operation_id = re.sub(r'[^A-Za-z0-9._-]', '_', operation_id)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason to strip everything instead of just the illegal characters?

*/:\"<|>?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Nope, I am just a noob and this is the first idea I got that solved it for me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In that case, welcome, and thank you for the contribution. =)

@Kosinkadink
Copy link
Copy Markdown
Member

Yeah, for this one I'd say strip it so that the headers are not part of the name, will be much cleaner.

@Kosinkadink
Copy link
Copy Markdown
Member

I merged #10156 by @bigcat88 that removes the headers from the name and makes sure the name does not get too long. Thank you for your initial submission, it led to the issue getting fixed!

@Kosinkadink Kosinkadink closed this Oct 2, 2025
@deshudiosh
Copy link
Copy Markdown
Author

I merged #10156 by @bigcat88 that removes the headers from the name and makes sure the name does not get too long. Thank you for your initial submission, it led to the issue getting fixed!

Thank you all, nice to be a part of it, even the smallest one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Good PR This PR looks good to go, it needs comfy's final review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants