Skip to content

task_110926_14.md

Latest commit

 

History

History
89 lines (58 loc) · 8.19 KB

task_110926_14.md

File metadata and controls

89 lines (58 loc) · 8.19 KB
Raw

Task: Harden feat/din-sdk / feat/din-daemon Before Merge — BL-19, BL-20, BL-21, BL-22

ID: task_110926_14 Assigned to: Santiago Rodriguez Cetran (@Santiagocetran) Created: 2026-09-11 Status: Open Repo: https://github.com/InfiniteZeroFoundation/DevNet Base branches: feat/din-sdk (PR #31, draft) for the NonceManager fix · feat/din-daemon (PR #32, draft, stacked on feat/din-sdk) for the other three Roadmap ref: P4-1.1 / P4-1.2 hardening Tracking: Developer/BACK_LOG.md BL-19, BL-20, BL-21, BL-22 · flagged in the PR #31/#32 code reviews (2026-09-10) and the P4 merging discussion #139

Context

Four real bugs were found during the deep-verification review of your own PR #31/#32 branches on 2026-09-10 (re-confirmed directly against the current branch heads — feat/din-sdk @ dc83f92, feat/din-daemon @ 8c20047 — as of 2026-09-11, nothing has changed since). All four are self-contained fixes inside code you already wrote, on branches you already own.

Blockers check (read before starting)

None. All four fixes touch only dincli/sdk/tx.py, dincli/dind/process.py, dincli/dind/main.py, dincli/dind/health.py, and dincli/dind/logging.py — files that already exist, on branches you already own, with tests you already wrote passing (248/248 on feat/din-daemon, 406/407 on feat/din-sdk as of the 2026-09-10 review — the one failure is unrelated, tracked separately). Nothing here needs input from issue #20 or #21's broader design, from Umer, or from any other contributor's branch. This is entirely yours to pick up and close.

BL-19 — dind start PID-file TOCTOU race (feat/din-daemon)

start() in dincli/dind/main.py does:

existing_pid = read_pid(paths.pid_path)
if existing_pid is not None and is_process_running(existing_pid):
    ...
    raise typer.Exit(1)
...
write_pid(paths.pid_path)

with no lock tying the check to the write — dincli/dind/process.py's read_pid/write_pid are plain file I/O, no O_EXCL, no flock. Two concurrent dind start invocations against the same --state-dir (systemd Restart=on-failure retry racing a manual restart, or a double-invoked deploy script) both pass the check and both start clean — the second silently orphans an unstoppable daemon doubling job-claim traffic against the same SQLite DB. Reproduced live during the 2026-09-10 review.

Fix: make the check-then-write atomic. The standard pattern is opening the PID file with os.O_CREAT | os.O_EXCL (fails atomically if the file already exists) instead of checking existence first — or an fcntl.flock()-held lock file that start() holds for its lifetime and status/stop can probe non-blockingly. Either is acceptable; pick whichever composes better with the existing StateDirs/PID-path structure. The current stale-PID cleanup path (remove_pid before writing, when the file exists but the PID inside is dead) still needs to keep working — don't regress that while fixing the race.

Test: a regression test that actually races two start() calls (threads or subprocesses against the same --state-dir) and asserts exactly one succeeds — not just a unit test of the lock primitive in isolation, since the bug was specifically about the race window.

BL-20 — container healthcheck can't detect degraded (feat/din-daemon)

HealthHandler.do_GET() in dincli/dind/health.py:

try:
    payload = self._build_payload()
    body = json.dumps(payload).encode("utf-8")
    self.send_response(200)
    ...
except Exception:
    ...
    self.send_error(500)

send_response(200) fires unconditionally as long as _build_payload() doesn't raise — but _build_payload() itself computes status: "degraded" (stale last_tick, event loop hung while the health thread still answers) and only puts it in the JSON body, never the status code. docker-compose.yml's curl -f http://127.0.0.1:8787/health only fails on non-2xx, so a daemon whose event loop has silently hung passes the healthcheck forever — undercutting the "Docker auto-restarts an unhealthy daemon" claim in the container handoff docs.

Fix: map status: "degraded" to a non-2xx response code (503 is the conventional choice — "temporarily unhealthy") while keeping status: "healthy" at 200. Update docker-compose.yml's healthcheck comment/docs if they currently claim more than crash-detection. Decide and document explicitly whether degraded should also trigger the container's restart policy (probably not — restart is for a genuinely stopped process, and degraded likely means "still ticking, just slow" — but that's your call to make and write down, not silently pick one way).

Test: a test that drives _build_payload()/do_GET() into the degraded branch (stale last_tick written into the state store) and asserts the HTTP status code, not just the JSON body.

BL-21 — dind persists no logs to disk (feat/din-daemon)

configure_logging() in dincli/dind/logging.py attaches a plain StreamHandler (stderr) with JsonFormatter — nothing is written under StateDirs, despite it already holding jobs.db/dind.pid/preferences.json. Under systemd (journald) or a container log driver, the supervisor persists logs; run directly in a terminal — the documented, tested path — stderr is gone when the session closes, leaving nothing to grep for "why did my node skip a job three hours ago," which matters for the non-expert operators issue #21 is aimed at.

Fix: add a RotatingFileHandler (stdlib logging.handlers) writing under StateDirs (e.g. <state_dir>/dind.log), alongside the existing stderr handler — don't remove stderr, systemd/container users still want it there. Pick sane defaults (e.g. 10MB × 5 backups) and make them overridable via env var, following the same DIN_DIND_* env-var convention already used for --state-dir/health host/port. Keep configure_logging() idempotent (it already is for the stream handler — don't double-attach the file handler on repeated calls either).

Test: start the daemon (or call configure_logging() directly), log a message, assert it landed in the file under the given state dir.

BL-22 — NonceManager._instances never evicted (feat/din-sdk)

NonceManager in dincli/sdk/tx.py keys its per-(chain_id, address) cache in a class-level _instances dict with no eviction — correct and irrelevant for the CLI (one invocation per process), but dind is long-lived, so this is unbounded process-wide state for its whole run. Already caused real test pollution during task_300726_8: reservations leaked between test classes until each cleared NonceManager._instances in setup_method.

Fix: doesn't need to be fancy — an LRU-style bound (e.g. a simple max-size dict with oldest-eviction) is enough given the realistic cardinality (one daemon manages a small, bounded number of accounts × networks, not an unbounded set). Document the bound and why it's safe. If you'd rather thread an explicit teardown/reset instead of a cap, that's fine too — just make sure it's real cleanup, not another place tests have to remember to reset manually (fix the root cause the existing test workaround is compensating for, don't just carry it forward).

Test: a test that exercises enough (chain_id, address) pairs to trigger the bound/eviction and asserts memory doesn't grow unboundedly — not just that the existing manual-reset test-pollution workaround still passes.

Definition of done

  • All four fixes landed as commits on feat/din-sdk (BL-22) / feat/din-daemon (BL-19/20/21), each with the regression test described above.
  • Full suite still green on both branches — no regressions from the fixes themselves.
  • Update Developer/BACK_LOG.md's BL-19/20/21/22 rows once each lands (status + which commit, same convention as the rest of the table).
  • Short PR-comment or commit-message pointer back to this task ID (task_110926_14) so the history is traceable, same convention as your other tasks.

No review sign-off needed to start — these are yours to just go fix. Flag here (or update PR #31/#32 directly) once done so the review can note the updated status.