task_110926_14 — Harden feat/din-sdk / feat/din-daemon: BL-19/20/21/22 (Santiago)

[umermjd11]

cc @Santiagocetran
Assigned. Full spec: Developer/tasks/task_110926_14.md

The task file has the full specification — exact code references, the fix for each item, and the regression test each one needs. This post is the summary and assignment notice.

Summary

Four real bugs found during the deep-verification review of your own PR #31/#32 branches (2026-09-10), tracked as Developer/BACK_LOG.md BL-19/20/21/22, re-confirmed against the current branch heads as of 2026-09-11:

• BL-19 — dind start's PID-file check-then-write isn't atomic; two concurrent start calls against the same --state-dir both succeed, orphaning an unstoppable duplicate daemon. Reproduced live during review.
• BL-20 — /health always returns HTTP 200 even when the computed status is "degraded" (stale tick) — the container healthcheck can't detect a hung event loop, only a crash.
• BL-21 — dind logs to stderr only; nothing persists under StateDirs, so a session run in a terminal loses all logs on exit.
• BL-22 — NonceManager._instances is an unbounded, never-evicted class-level cache — fine for the CLI's one-shot process, not for a long-lived daemon.

Blockers

None. All four touch only files you already own on branches you already own (dincli/sdk/tx.py, dincli/dind/{main,process,health,logging}.py), with your own tests currently green (248/248 on feat/din-daemon, 406/407 on feat/din-sdk). Nothing here needs sign-off, design input from anyone else, or another PR to land first — go ahead and start.

[Santiagocetran]

Hi @umermjd11 — this one's done, thanks for the clear write-up on each item, the reproduction notes made them quick to confirm.

All four are fixed, each with the regression test the task asked for:

• BL-19 — atomic start via flock on a separate lock file. Chose that over O_EXCL because the kernel releases an flock on process death including SIGKILL, so a hard kill doesn't leave a stale file behind. status and stop use the lock as evidence now too, so stop won't signal an unrelated process that reused a stale PID.
• BL-20 — degraded answers 503. dind status needed updating in the same commit or a degraded daemon would have read as unreachable. Also corrected the auto-restart claim in three places: it was never true, since Compose restarts on container exit and only Swarm restarts on health status. Wrote the decision down — degraded reports, it doesn't restart.
• BL-21 — rotating JSON logs under the state dir, alongside stderr.
• BL-22 — bounded cache, plus terminal nonce cleanup first. Confirmed and reverted nonces never left _inflight, so a bound on its own would have read completed work as busy and started refusing new accounts.

Two notes. The state dir holds dind.db, not jobs.db — the task file and the BL-21 row both say jobs.db, so I've corrected the backlog row. And the BL-19 race test needs a test-only delay hook in dind start, inert unless its env var is set: my first version passed even with the lock removed, so the hook is what makes it an actual regression test. It now fails 15/15 with the lock neutered and passes 15/15 restored.

Backlog rows updated for all four. Branch is pushed, suite at 620 passed, 0 failed.

[Santiagocetran]

Update on this one — I ran a deeper review over the delivered work and it turned up a few things I want to fix before treating it as closed.

Two on BL-22: the bounded-cache fix doesn't cover a manager that's evicted while idle and then reserved against — once the caller drops its reference the reservation goes with it, so the next lookup can hand out the same nonce. Reproduced it directly. And my note above about stop was stronger than what the code guarantees: reading the PID and probing the lock are separate observations, so a daemon replacement in between can still lead to signalling the old PID. BL-20 and BL-21 hold up.

Both PRs are in draft while I work through it. I'll re-report once fixed and verified.

[Santiagocetran]

Following up on my correction above — all of it is fixed now.

BL-22 took three passes, which is worth recording. The bound alone wasn't the problem: confirmed and reverted nonces never left _inflight, so a bound would have read completed work as busy. Fixing that exposed a second issue, and the fix for that rested on a false assumption — that a manager nobody references holds no reservations. It does: build_tx_params() hands back the nonce as a plain integer and drops the manager, so a manager evicted while idle could be reserved against and then collected with its reservation. Now an activity lease keeps nonce state alive independently of whoever holds a reference (225a46d), with the 64-entry bound unchanged.

BL-19 — the stop claim I made was worse than too strong. stop was signalling whatever PID was on file as soon as the lock looked held, with no identity check at all; we demonstrated it by having it kill the test process. There's now a same-user control socket carrying a per-instance id, so stop and status verify which daemon they reached instead of signalling (e4080f7). That's new production code and new surface, so it's flagged in #32 rather than buried — including what it doesn't guarantee. Also fixed start() refusing to start because a stale PID had been reused by an unrelated live process, despite holding the exclusive lock.

BL-20 and BL-21 stood up to the second review unchanged.

Backlog rows now name the commits that actually fixed each item rather than the first attempt — BL-22's says "resolved on the third attempt" and explains why the first two weren't enough, so it doesn't read later as though adding a bound was the whole story.

[umeradl]

Heads-up: dincli dindao is now dincli dinrep — PR No. 175 landed on develop (merge 5864f7d, follow-up 8f9a90b). The DIN-Representative command group is renamed to match the role: dincli/cli/dindao.py → dincli/cli/dinrep.py, Documentation/public/roles/dindao.md → dinrep.md. Breaking, no alias — dincli dindao … now returns "No such command". Subcommands are unchanged (dincli dinrep deploy …, dincli dinrep add-slasher …, dincli dinrep registry …).

For this task: no code conflict (PR No. 31 / No. 32 don't modify dindao.py), but their docs refer to dindao.py (e.g. dindao.py::list_pending_requests, the deploy-flow note on tx_receipt.contractAddress). Please update those references to dinrep.py when you next merge develop.

[Santiagocetran]

Addressed in PRs #31 and #32. Both branches now include the latest develop, the dinrep rename, and updated SDK documentation references. Related compatibility fixes are included; unit tests and local-chain registry/fee-sweep checks pass.