Skip to content

feat(registry): 注册中心日志记录到文件,添加日志轮转配置 - #23

Open
openjiuwen-sync-bot[bot] wants to merge 1 commit into
openJiuwen-ai:feature/Agentregistry-devfrom
openjiuwenai:sync/pr-277
Open

feat(registry): 注册中心日志记录到文件,添加日志轮转配置#23
openjiuwen-sync-bot[bot] wants to merge 1 commit into
openJiuwen-ai:feature/Agentregistry-devfrom
openjiuwenai:sync/pr-277

Conversation

@openjiuwen-sync-bot

@openjiuwen-sync-bot openjiuwen-sync-bot Bot commented Aug 12, 2026

Copy link
Copy Markdown

Paired: GitHub #23GitCode !277

注册中心日志记录到文件,添加日志轮转配置
需要配合安装脚本的修改才能生效
如果没有配置日志路径,则需要用journalctl查看日志

What type of PR is this?

/kind

Self-checklist:(请自检,在[ ]内打上x,我们将检视你的完成情况,否则会导致pr无法合入

    • 设计:PR对应的方案是否已经经过Maintainer评审,方案检视意见是否均已答复并完成方案修改
    • 测试:PR中的代码是否已有UT/ST测试用例进行充分的覆盖,新增测试用例是否随本PR一并上库或已经上库
    • 验证:PR描述信息中是否已包含对该PR对应的Feature、Refactor、Bugfix的预期目标达成情况的详细验证结果描述
    • 接口:是否涉及对外接口变更,相应变更已得到接口评审组织的通过,API对应的注释信息已经刷新正确
    • 文档:是否涉及官网文档修改,如果涉及请及时提交资料到Doc仓

@openjiuwen-collaboration-bot

openjiuwen-collaboration-bot Bot commented Aug 12, 2026

Copy link
Copy Markdown

head_sha: 4cbf2fb56c805a4059acbee0a1199466cf351ff5

变更摘要

此 PR 为注册中心后端添加了日志文件记录与每日轮转能力。核心改动包括:新增 DailyCompressedFileHandler 日志处理器,支持按天轮转并将旧日志 gzip 压缩;通过环境变量 A2X_REGISTRY_LOG_DIRA2X_REGISTRY_LOG_RETENTION_DAYS 控制日志目录与保留天数;在 _serve 启动流程中统一配置日志,使 uvicorn 与应用日志同时输出到 stderr 和可选的文件。

主要改动

  • 新增 DailyCompressedFileHandler 日志处理器:继承自 TimedRotatingFileHandler,每日午夜轮转时将当日日志 gzip 压缩(.log.gz),并自动清理超出保留天数的旧文件,当前日志文件以 a2x-registry-YYYY-MM-DD.log 命名。
  • 新增环境变量配置A2X_REGISTRY_LOG_DIR 指定日志目录(为空则仅输出到 stderr),A2X_REGISTRY_LOG_RETENTION_DAYS 控制压缩日志保留天数(默认 7,最小为 1),解析时含错误校验。
  • RuntimeConfig 扩展:新增 log_dir: strlog_retention_days: int = 7 字段,parse_runtime_config 中完成环境变量取值与合法性校验。
  • 新增 _configure_logging 函数:统一配置 root logger,始终保留 StreamHandler(sys.stderr),若 log_dir 非空则追加 DailyCompressedFileHandler,format 为 %(asctime)s [%(levelname)s] %(name)s: %(message)s
  • _serve 启动流程集成:在 uvicorn.run 前调用 _configure_logging(cfg),并传入 log_config=None,确保 uvicorn 内部日志(uvicorn / uvicorn.error / uvicorn.access)沿用 root logger 配置。

@openjiuwen-collaboration-bot

openjiuwen-collaboration-bot Bot commented Aug 12, 2026

Copy link
Copy Markdown

head_sha: 4cbf2fb56c805a4059acbee0a1199466cf351ff5

代码审查

Good — Python >= 3.10 is required. So force=True in logging.basicConfig is fine (added in 3.8). from __future__ import annotations is also fine (added in 3.7).

Now let me continue my analysis.

The Python version is >= 3.10, so:

  • force=True in logging.basicConfig — OK (3.8+)
  • from __future__ import annotations — OK (3.7+)
  • shutil.copyfileobj — standard library
  • gzip — standard library
  • pathlib.Path — standard library

All good.

Now let me focus on the actual issues in the diff:

File 1: __main__.py

Let me re-examine the _configure_logging function and the call site:

def _configure_logging(cfg: RuntimeConfig) -> None:
    handlers: list = [logging.StreamHandler(sys.stderr)]
    if cfg.log_dir:
        log_dir = Path(cfg.log_dir)
        log_dir.mkdir(parents=True, exist_ok=True)
        handlers.append(
            DailyCompressedFileHandler(log_dir, "a2x-registry", cfg.log_retention_days)
        )
    logging.basicConfig(
        level=logging.INFO,
        format="%(asctime)s [%(levelname)s] %(name)s: %(message)s",
        handlers=handlers,
        force=True,
    )

And in _serve:

    import uvicorn
    _configure_logging(cfg)
    uvicorn.run(
        "a2x_registry.backend.app:app",
        host=cfg.bind,
        port=port,
        reload=args.reload,
        timeout_keep_alive=args.keep_alive,
        log_config=None,  # keep our root-level config (stderr + optional file)
        **ssl_kwargs,
    )

One issue I see: _configure_logging is called INSIDE _serve, after the import uvicorn statement. This means if import uvicorn already configures logging (which uvicorn might do), there could be a conflict. But force=True would override it.

Actually, import uvicorn might trigger uvicorn's own logging configuration. Let me think... In uvicorn, importing the module doesn't configure logging. Logging is configured when uvicorn.run() is called. So the order is:

  1. import uvicorn — no logging config
  2. _configure_logging(cfg) — configures root logger
  3. uvicorn.run(log_config=None) — uses existing logging config

This should be fine.

Now let me focus on log.py more carefully.

File 2: log.py

The most interesting issue I see is in doRollover:

def doRollover(self) -> None:
    if self.stream:
        self.stream.close()
        self.stream = None
    cur = Path(self.baseFilename)
    if cur.exists():
        gz_path = Path(str(cur) + ".gz")
        with open(cur, "rb") as f_in, gzip.open(gz_path, "wb") as f_out:
            shutil.copyfileobj(f_in, f_out)
        cur.unlink()
    # Open a fresh file stamped with today's date, then prune old .gz.
    self.baseFilename = str(self._log_dir / self._dated_name(date.today()))
    self.stream = self._open()
    self._prune()
    self.rolloverAt = self.computeRollover(time.time())

Key issues:

  1. Exception during gzip/copy leaves handler broken: If open, gzip.open, or copyfileobj raises an exception:

    • self.stream is None
    • self.baseFilename is unchanged
    • A partial .gz file may exist
    • Exception propagates to the caller (the logging emit method)

    This means that a single disk-full or permission error during rollover can break the entire logging system. The next emit call would try rollover again (since rolloverAt wasn't updated), and would fail again. This is a P2 issue.

  2. If cur.unlink() fails: We'd have both file.log and file.log.gz. This is minor, but the orphaned .log file will never be cleaned up by _prune since it only matches .log.gz. P3.

  3. The _prune method:

    def _prune(self) -> None:
        files = sorted(
            self._log_dir.glob(f"{self._stem}-*.log.gz"),
            key=lambda p: p.name,
        )
        for old in files[: max(0, len(files) - self._retention_days)]:
            try:
                old.unlink()
            except OSError:
                pass

    The except OSError: pass is appropriate for a best-effort cleanup. But note: if _retention_days is 0 (which can't happen due to validation in parse_runtime_config, but could if someone calls DailyCompressedFileHandler directly), ALL files would be deleted. This is a minor defense-in-depth concern.

  4. The _prune retention logic has an off-by-one: The _prune keeps retention_days .log.gz files. The current .log file is not counted. So with retention_days=7 and log_dir set, you get: current .log + 7 .log.gz = 8 days of logs. But the env var is named A2X_REGISTRY_LOG_RETENTION_DAYS, which users might interpret as total days of logs. The docstring says "how many daily-rotated files to keep" but the env var says "retention days". This is a documentation/UX ambiguity, not a bug. But worth noting.

Actually wait, let me re-read the env var description in the diff:

    A2X_REGISTRY_LOG_RETENTION_DAYS
                             daily-rotated .gz files to keep (default 7)

This says "daily-rotated .gz files to keep" — which matches the implementation (keeps 7 .gz files). So the doc is correct. The env var name might be slightly misleading (suggesting total days), but the comment clarifies it. Not a bug.

Now let me think about one more thing: the _prune method is called inside doRollover, which is called with the handler's lock held (by the parent's emit). If _prune takes a long time (e.g., many files on a slow filesystem), it could block logging. But this is unlikely to be a real issue.

Let me now think about whether there are any issues I'm missing...

Actually, I want to look more carefully at _configure_logging:

handlers: list = [logging.StreamHandler(sys.stderr)]

The list type hint without subscript. With from __future__ import annotations, this won't be evaluated at runtime, but static type checkers would flag it. This is a minor style issue. P3.

But wait — there is a potential issue with logging.basicConfig(handlers=handlers, force=True). When force=True is used, basicConfig removes all existing handlers from the root logger and adds the new ones. But what about loggers that have propagate=False? Their behavior is unaffected. For loggers that propagate to root, the new handlers will receive their messages. This is correct behavior.

But there's another subtle issue: _configure_logging is called from _serve. If _serve raises an exception before uvicorn.run(), the logging is already configured. This is fine.

However, what about the main() function? It dispatches to auth_main or cluster_main or _serve. _configure_logging is only called in _serve. If someone calls main()auth_main, logging is NOT configured (it uses Python's default). This is probably intentional since auth subcommands don't need file logging.

Let me now check: is there an issue with the _prune and _retention_days that could delete ALL files?

for old in files[: max(0, len(files) - self._retention_days)]:

If files has 10 entries and _retention_days is 7: max(0, 10 - 7) = 3, deletes first 3, keeps last 7. Correct.

If files has 3 entries and _retention_days is 7: max(0, 3 - 7) = 0, deletes none. Correct.

The max(0, ...) guard prevents negative indexing. Good.

One more thing: What about the interaction between doRollover and _prune?

In doRollover:

  1. Close current stream
  2. Gzip the current file → file.log.gz
  3. Delete current file
  4. Open new file with today's date
  5. Call _prune

At step 5, the newly created .gz file (from step 3) already exists. _prune globs for *.log.gz, which matches this new file PLUS all old files. Then it deletes the oldest ones, keeping retention_days files. Since the new .gz was just created and has the most recent date (well, yesterday's date because it was the file that just rolled over), it would be among the kept files.

Wait — actually, the new .gz file has the date from self.baseFilename before the rollover. That date is yesterday (or earlier if rollover was delayed). When sorted by name, it could be anywhere in the list. If _retention_days is small and there are many old files, the newly gzipped file could be deleted! Let me trace through this:

Scenario: retention_days=1, and we have files:

  • a2x-registry-2026-01-01.log.gz (old)
  • a2x-registry-2026-01-02.log (current, being rolled over)

After rollover:

  • a2x-registry-2026-01-01.log.gz (old)
  • a2x-registry-2026-01-02.log.gz (just gzipped)
  • a2x-registry-2026-01-03.log (new, current day)

_prune finds: ['a2x-registry-2026-01-01.log.gz', 'a2x-registry-2026-01-02.log.gz']
len(files) - retention_days = 2 - 1 = 1
Deletes files[:1] = ['a2x-registry-2026-01-01.log.gz']
Keeps: ['a2x-registry-2026-01-02.log.gz']

This is correct! The oldest file is deleted, the most recent .gz is kept.

But what if the rollover was delayed and self.baseFilename had a much older date? Then the newly gzipped file could be among the oldest and get deleted immediately. But that's actually what should happen in that case — the data is very old.

Actually wait, let me reconsider. The _prune should probably exclude the file that was JUST created. But in practice, since _retention_days is 7 by default, and rollover happens daily, there should be at most _retention_days or _retention_days+1 .gz files at any time. The just-created .gz file would have yesterday's date (or the date of the last rollover), which is the most recent among all .gz files. So it would be kept. Only in the edge case where rollover was delayed by days would this be an issue, and even then, the behavior is arguably correct.

I think the implementation is actually sound in this regard.

Let me now look for issues I might have missed...

Actually, let me look at line 46 in log.py more carefully:

with open(cur, "rb") as f_in, gzip.open(gz_path, "wb") as f_out:
    shutil.copyfileobj(f_in, f_out)

shutil.copyfileobj copies in chunks (default 16KB). For large log files, this could take a while. During this time, the handler's lock is held, blocking all logging. This is a potential performance issue with very large log files. But since rollover happens daily, log files shouldn't be enormous. P3.

Also, I notice that shutil.copyfileobj doesn't preserve file metadata (permissions, timestamps). This is probably fine for log files.

One more check: is _open() called correctly?

self.stream = self._open()

_open() is defined in the parent class BaseRotatingHandler (or TimedRotatingFileHandler). It opens self.baseFilename in append mode with the specified encoding. Since self.baseFilename was just set and the file was just created (new file, or overwritten), this should work.

But what if self._open() fails? The stream would remain None, and the next emit would try to open it again. This is handled by the parent class's emit method which checks if self.stream is None.

Now let me look for any issues in __main__.py more carefully...

In _serve:

import uvicorn
_configure_logging(cfg)
uvicorn.run(
    ...
    log_config=None,  # keep our root-level config (stderr + optional file)
    ...
)

One potential issue: if _configure_logging raises an exception, uvicorn.run is never called. But _configure_logging only raises if log_dir.mkdir() fails (permissions), or if DailyCompressedFileHandler.__init__() fails. Both are unlikely but possible. If they fail, the error message would go to... well, stderr via Python's default logging, which is what we want.

Actually wait — _configure_logging configures logging. If the directory creation fails, the basicConfig hasn't been called yet (it's after the directory check). So logging would use Python's defaults. But the exception would propagate, and the process would exit with a traceback on stderr. This is acceptable.

But there's a subtle ordering issue: _configure_logging calls logging.basicConfig AFTER possibly raising an exception. If log_dir.mkdir() succeeds but DailyCompressedFileHandler(log_dir, ...) raises (e.g., permission denied on the log file), basicConfig hasn't been called yet. The exception propagates, and logging is in its default state. The traceback goes to stderr. Acceptable.

OK, I think I've done a thorough analysis. Let me now report the findings.

Let me also quickly check — is there a __init__.py in the common directory?

From the glob results, yes: AgentRegistry/a2x_registry/common/__init__.py exists.

Let me also check if the common/__init__.py exports DailyCompressedFileHandler or if there's any import issue.

Actually, the import in __main__.py is:

from a2x_registry.common.log import DailyCompressedFileHandler

This imports directly from the log module, not from a2x_registry.common. So __init__.py doesn't need to export it. Fine.

Now let me summarize the issues I've found:

P2 issues:

  1. log.py doRollover: exception during gzip leaves handler in broken state — If gzip.open, open, or copyfileobj fails (disk full, permission error), self.stream is None, self.baseFilename unchanged, rolloverAt is NOT updated, and exception propagates. Subsequent logging calls will retry rollover and fail again. This effectively breaks the file logging handler. A partial .gz file may also be left behind.

  2. log.py doRollover: orphaned .log file if unlink fails — If cur.unlink() fails (e.g., permission error), the old .log file is never cleaned up and won't be matched by _prune's glob pattern.

P3 issues:

  1. __main__.py: handlers: list type hint without subscript — Minor typing issue.

  2. log.py _prune: except OSError: pass silently swallows deletion errors — If files can't be deleted due to permissions, they accumulate silently.

Let me now look more carefully at the _prune issue. The except OSError: pass could mask real disk issues. But this is a design choice — log handlers should be best-effort. P3.

Let me also think about whether there are any security issues I've missed:

  • The log_dir from env var is used directly in Path(cfg.log_dir). If it contains .. sequences, it could write outside the intended directory. But this is a server configuration, not user input.
  • The stem parameter in DailyCompressedFileHandler is hardcoded as "a2x-registry" in _configure_logging.
  • No hardcoded secrets.

I think I've covered all the issues. Let me also double-check one thing: the _prune method's max(0, len(files) - self._retention_days) — this is defensive but correct. If len(files) <= retention_days, the max is 0, and files[:0] is empty, so nothing is deleted.

Wait, I want to re-examine one thing. In _prune:

for old in files[: max(0, len(files) - self._retention_days)]:

If len(files) = 10 and _retention_days = 7: deletes first 3 (keeps 7).
If len(files) = 10 and _retention_days = 10: deletes first 0 (keeps 10).
If len(files) = 10 and _retention_days = 11: max(0, 10-11) = 0, deletes none. Correct.

This is correct. The retention is "keep at most retention_days files", which matches the doc.

Now let me also check one more edge case: what if self._retention_days is very large (e.g., 1000000)? max(0, len(files) - 1000000) would be 0, so nothing is deleted. This is fine — no performance issue since files[:0] is empty.

OK, I'm confident in my analysis. Let me now report the findings.

Actually wait, let me re-examine one more subtle issue. In _configure_logging:

def _configure_logging(cfg: RuntimeConfig) -> None:
    handlers: list = [logging.StreamHandler(sys.stderr)]
    if cfg.log_dir:
        log_dir = Path(cfg.log_dir)
        log_dir.mkdir(parents=True, exist_ok=True)
        handlers.append(
            DailyCompressedFileHandler(log_dir, "a2x

| 类型 | 数量 |
|------|-------|
| 🔴 阻塞 | 1 |
| 🟡 建议 | 0 |需要修改

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@openjiuwen-collaboration-bot

Copy link
Copy Markdown

head_sha: 4cbf2fb56c805a4059acbee0a1199466cf351ff5

任务名称 结果 日志操作
静态检查 ✅SUCCESS 点此跳转
防投毒检查 ✅SUCCESS 点此跳转
开源合规检查 ✅SUCCESS 点此跳转

self.baseFilename = str(self._log_dir / self._dated_name(date.today()))
self.stream = self._open()
self._prune()
self.rolloverAt = self.computeRollover(time.time())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

head_sha: 4cbf2fb56c805a4059acbee0a1199466cf351ff5

🟠 High Priority

changed line → affected behavior/contract → failure mode → suggested fix

log.py 第 39-53 行 doRollover 方法:先关闭 stream(第 40-42 行),再执行 gzip 压缩(第 46-48 行)。如果 gzip 阶段因磁盘满、权限错误、I/O 错误等原因抛出异常,异常沿 emit()handleError() 传播,错误信息被打印到 stderr,但 self.stream 已为 None,且永远不会被重新打开。此后所有 emit() 调用中 self.stream.write(...) 触发 AttributeError,被 handleError 静默吞掉,直到下一次 rollover(~24 小时后)才会有新流。

触发条件:磁盘满是最现实的生产环境场景。日志文件可能很大,gzip 写压缩文件需要额外磁盘空间。
后果:后续最多 24 小时的日志全部静默丢失。运维只能从 stderr 看到一条 traceback,但无法恢复日志。

修复方向:在 doRollover 中用 try/finally 保护:gzip 步骤应放在 try 块中;无论成功与否,finally 块都必须重新打开一个新文件流并设置 self.rolloverAt,确保 handler 绝不会处于 stream 为 None 的状态。如果 gzip 失败,可记录 warning 并跳过压缩(旧 .log 文件保留在原地不压缩),然后正常打开当天新文件。

建议:用 try/finally 重构 doRollover:在 finally 块中确保无论 gzip 成功与否都重新打开新文件流并设置 rolloverAt。gzip 失败时记录 warning 并跳过压缩,保留原始 .log 文件。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

head_sha: 09b6b499d06b11887cda2d12faf7dd4ab88517f8

已修复

@openjiuwen-collaboration-bot

Copy link
Copy Markdown

head_sha: 09b6b499d06b11887cda2d12faf7dd4ab88517f8

任务名称 结果 日志操作
静态检查 ✅SUCCESS 点此跳转
防投毒检查 ✅SUCCESS 点此跳转
开源合规检查 ✅SUCCESS 点此跳转

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants