Constats mineurs de la review de la pile monitoring des postes #25

New issue

Closed

opened 2026-08-16 18:28:49 +00:00 by maximus · 0 comments

maximus commented

2026-08-16 18:28:49 +00:00

Owner

Constats non bloquants relevés par /pr-review sur la pile #18-#23 (monitoring des postes). Aucun n'est un défaut de comportement. Analyse du 2026-09-29 : les quatre prémisses ont été vérifiées dans le code, et deux décisions ont été tranchées par Max (points 1 et 3).

1. `loadAvg` : exiger exactement 3 nombres (décision : corriger)

Constat vérifié : sanitizeCpu() (index.js:348) accepte de 0 à 3 éléments et tronque au-delà. hosts.test.js:702 épingle [] comme accepté, et hosts.test.js:763 épingle [1,2,3,4,5] tronqué en [1,2,3]. Le consommateur (carte admin, la-compagnie-maximus#149) fait loadAvg.map(...).join(" / ").

Décision : 400 si loadAvg n'a pas exactement 3 nombres finis (message cpu.loadAvg must hold exactly 3 finite numbers). L'agent envoie os.loadavg(), qui a toujours 3 éléments ; tous les fixtures (agent.test.js, test-curl.sh) en ont déjà 3.

Inverser les deux tests cités : [] et [1..5] donnent 400, et rien n'est écrit. Ajouter les cas à 1 et à 2 éléments dans la matrice de validation (hosts.test.js:656).
Relecture : readHostSnapshot() repasse par sanitizeSnapshot(), donc un fichier déjà stocké avec moins de 3 éléments dégraderait en neverSeen. Aucun n'existe en pratique (l'agent envoie toujours 3). Ajouter un cas de dégradation qui le fige.
Contrat : README (section POST /hosts/<id>) et agent/README.md (section Payload) précisent loadAvg = exactement 3 nombres (1, 5 et 15 min).

2. `cleanString` conserve les caractères de contrôle (décision : documenter)

Constat vérifié : cleanString() (index.js:315) ne fait que tronquer à 128 caractères. C'est un choix assumé : ne pas mutiler une valeur légitime.

Commentaire sur cleanString(), plus une phrase dans le README à côté de « strings are capped at 128 chars » : les chaînes ne sont pas filtrées, et le consommateur doit les échapper au rendu. React le fait pour du texte, mais un rendu en dangerouslySetInnerHTML ou en attribut href ne serait pas sûr.

3. Débris `.tmp` après un crash en cours d'écriture (décision : balayer au démarrage)

Constat vérifié : writeHostSnapshot() (index.js:394) supprime le .tmp sur exception, mais un SIGKILL ou un OOM entre writeFileSync et renameSync laisse .<id>.<pid>.<ts>.tmp dans HOSTS_DIR. Rien ne le balaie. Sans conséquence fonctionnelle, puisque GET /hosts ne lit que <id>.json.

Nouvelle fonction exportée sweepStaleTempFiles(now). Elle supprime seulement les noms au format exact ^\.[a-z0-9][a-z0-9-]{0,31}\.\d+\.\d+\.tmp$ (même regex d'id que la route) dont le mtime date de plus d'une heure. Best effort : HOSTS_DIR absent, fichier illisible ou unlink en échec sont journalisés, jamais bloquants.
Appel au démarrage, dans le bloc require.main === module, avant listen.
Tests : un .tmp vieux est supprimé ; un .tmp récent est gardé (écriture concurrente possible) ; <id>.json et un fichier au nom hors format sont gardés, même vieux ; un HOSTS_DIR absent ne lève pas d'exception.
CLAUDE.md (Gotchas, écriture atomique) : mentionner le balayage.

4. `frozenRequest` ne fige pas `new Date()` (décision : documenter)

Constat vérifié : le commentaire de frozenRequest (hosts.test.js:138) dit déjà que seul Date.now est remplacé, mais il n'en tire pas la conséquence concrète. POST /hosts date receivedAt avec new Date().toISOString() (index.js:633), qui ne passe pas par Date.now. Un POST sous frozenRequest reçoit donc l'heure réelle, pas FROZEN_NOW.

Compléter le commentaire : seuls Date.now() et ce qui en dérive sont figés ; new Date() sans argument lit l'horloge réelle, y compris le receivedAt d'un POST. Pour un receivedAt contrôlé, écrire le fichier via writeSnapshotAged().

Fichiers concernés

index.js : sanitizeCpu, cleanString (commentaire), sweepStaleTempFiles (nouvelle), bloc de démarrage, exports
__tests__/hosts.test.js : tests loadAvg inversés et étendus, dégradation en relecture, tests du balayage, commentaire de frozenRequest
README.md, agent/README.md : contrat loadAvg, chaînes non filtrées
CLAUDE.md : nombre de tests, Gotchas (balayage .tmp, loadAvg)

Critères d'acceptation

loadAvg avec 0, 1, 2 ou 4+ éléments donne 400, rien n'est écrit ; 3 éléments donnent 204
Un snapshot stocké avec un loadAvg hors format dégrade en neverSeen à la relecture
sweepStaleTempFiles supprime seulement les .tmp au bon format et vieux de plus d'une heure, et ne lève jamais d'exception
Le balayage est appelé au démarrage (require.main), avant listen
Les choix de cleanString et de frozenRequest sont documentés avec leur raison
Contrat mis à jour dans README et agent/README ; npm test, bash test-curl.sh et sh scripts/boot-from-copy.sh restent verts
Chaque nouveau test est prouvé rouge par mutation (retour à slice(0, 3) ; filtre d'âge ou de nom retiré)

Complexité

Simple à moyenne. Pas de nouvelle variable d'environnement, pas de nouveau module runtime (le COPY du Dockerfile ne change pas). Un redéploiement est nécessaire (changement de index.js) ; l'agent déjà installé envoie 3 éléments, donc il n'y a aucune rupture côté poste.

Contrôle de dérive : l'issue date du 2026-08-16, mais tous les symboles cités existent dans ce repo.

Constats non bloquants relevés par `/pr-review` sur la pile #18-#23 (monitoring des postes). Aucun n'est un défaut de comportement. Analyse du 2026-09-29 : les quatre prémisses ont été vérifiées dans le code, et deux décisions ont été tranchées par Max (points 1 et 3). ## 1. `loadAvg` : exiger exactement 3 nombres (décision : corriger) Constat vérifié : `sanitizeCpu()` (`index.js:348`) accepte de 0 à 3 éléments et tronque au-delà. `hosts.test.js:702` épingle `[]` comme accepté, et `hosts.test.js:763` épingle `[1,2,3,4,5]` tronqué en `[1,2,3]`. Le consommateur (carte admin, `la-compagnie-maximus#149`) fait `loadAvg.map(...).join(" / ")`. Décision : **400 si `loadAvg` n'a pas exactement 3 nombres finis** (message `cpu.loadAvg must hold exactly 3 finite numbers`). L'agent envoie `os.loadavg()`, qui a toujours 3 éléments ; tous les fixtures (`agent.test.js`, `test-curl.sh`) en ont déjà 3. - Inverser les deux tests cités : `[]` et `[1..5]` donnent 400, et rien n'est écrit. Ajouter les cas à 1 et à 2 éléments dans la matrice de validation (`hosts.test.js:656`). - Relecture : `readHostSnapshot()` repasse par `sanitizeSnapshot()`, donc un fichier déjà stocké avec moins de 3 éléments dégraderait en `neverSeen`. Aucun n'existe en pratique (l'agent envoie toujours 3). Ajouter un cas de dégradation qui le fige. - Contrat : README (section `POST /hosts/<id>`) et `agent/README.md` (section Payload) précisent `loadAvg` = exactement 3 nombres (1, 5 et 15 min). ## 2. `cleanString` conserve les caractères de contrôle (décision : documenter) Constat vérifié : `cleanString()` (`index.js:315`) ne fait que tronquer à 128 caractères. C'est un choix assumé : ne pas mutiler une valeur légitime. - Commentaire sur `cleanString()`, plus une phrase dans le README à côté de « strings are capped at 128 chars » : les chaînes ne sont pas filtrées, et le consommateur doit les échapper au rendu. React le fait pour du texte, mais un rendu en `dangerouslySetInnerHTML` ou en attribut `href` ne serait pas sûr. ## 3. Débris `.tmp` après un crash en cours d'écriture (décision : balayer au démarrage) Constat vérifié : `writeHostSnapshot()` (`index.js:394`) supprime le `.tmp` sur exception, mais un SIGKILL ou un OOM entre `writeFileSync` et `renameSync` laisse `.<id>.<pid>.<ts>.tmp` dans `HOSTS_DIR`. Rien ne le balaie. Sans conséquence fonctionnelle, puisque `GET /hosts` ne lit que `<id>.json`. - Nouvelle fonction exportée `sweepStaleTempFiles(now)`. Elle supprime seulement les noms au format exact `^\.[a-z0-9][a-z0-9-]{0,31}\.\d+\.\d+\.tmp$` (même regex d'id que la route) dont le mtime date de plus d'une heure. Best effort : `HOSTS_DIR` absent, fichier illisible ou `unlink` en échec sont journalisés, jamais bloquants. - Appel au démarrage, dans le bloc `require.main === module`, avant `listen`. - Tests : un `.tmp` vieux est supprimé ; un `.tmp` récent est gardé (écriture concurrente possible) ; `<id>.json` et un fichier au nom hors format sont gardés, même vieux ; un `HOSTS_DIR` absent ne lève pas d'exception. - `CLAUDE.md` (Gotchas, écriture atomique) : mentionner le balayage. ## 4. `frozenRequest` ne fige pas `new Date()` (décision : documenter) Constat vérifié : le commentaire de `frozenRequest` (`hosts.test.js:138`) dit déjà que seul `Date.now` est remplacé, mais il n'en tire pas la conséquence concrète. `POST /hosts` date `receivedAt` avec `new Date().toISOString()` (`index.js:633`), qui ne passe pas par `Date.now`. Un POST sous `frozenRequest` reçoit donc l'heure réelle, pas `FROZEN_NOW`. - Compléter le commentaire : seuls `Date.now()` et ce qui en dérive sont figés ; `new Date()` sans argument lit l'horloge réelle, y compris le `receivedAt` d'un POST. Pour un `receivedAt` contrôlé, écrire le fichier via `writeSnapshotAged()`. ## Fichiers concernés - `index.js` : `sanitizeCpu`, `cleanString` (commentaire), `sweepStaleTempFiles` (nouvelle), bloc de démarrage, exports - `__tests__/hosts.test.js` : tests `loadAvg` inversés et étendus, dégradation en relecture, tests du balayage, commentaire de `frozenRequest` - `README.md`, `agent/README.md` : contrat `loadAvg`, chaînes non filtrées - `CLAUDE.md` : nombre de tests, Gotchas (balayage `.tmp`, `loadAvg`) ## Critères d'acceptation - [ ] `loadAvg` avec 0, 1, 2 ou 4+ éléments donne 400, rien n'est écrit ; 3 éléments donnent 204 - [ ] Un snapshot stocké avec un `loadAvg` hors format dégrade en `neverSeen` à la relecture - [ ] `sweepStaleTempFiles` supprime seulement les `.tmp` au bon format et vieux de plus d'une heure, et ne lève jamais d'exception - [ ] Le balayage est appelé au démarrage (`require.main`), avant `listen` - [ ] Les choix de `cleanString` et de `frozenRequest` sont documentés avec leur raison - [ ] Contrat mis à jour dans README et agent/README ; `npm test`, `bash test-curl.sh` et `sh scripts/boot-from-copy.sh` restent verts - [ ] Chaque nouveau test est prouvé rouge par mutation (retour à `slice(0, 3)` ; filtre d'âge ou de nom retiré) ## Complexité Simple à moyenne. Pas de nouvelle variable d'environnement, pas de nouveau module runtime (le `COPY` du Dockerfile ne change pas). Un redéploiement est nécessaire (changement de `index.js`) ; l'agent déjà installé envoie 3 éléments, donc il n'y a aucune rupture côté poste. Contrôle de dérive : l'issue date du 2026-08-16, mais tous les symboles cités existent dans ce repo.