[Feature] - Répare et accélère l'import RPPS, passage à une passe complète hebdomadaire - #60
Conversation
…olonnes de l'import Doctrine ORM 3 a supprimé l'argument `indexes` de #[ORM\Table] et l'ignore silencieusement. Tous les index déclarés ainsi étaient donc inertes : City, RPPS, RPPSAddress et Specialty. CCAM était la seule entité à utiliser la forme supportée (un #[ORM\Index] au niveau de la classe), et c'est le seul index qui apparaît réellement dans un schéma généré. Conséquence en production : la table city n'a aucun index sur postal_code, alors que RPPSService::findCityEntity() exécute un `SELECT ... FROM city WHERE postal_code = ?` par ligne d'adresse importée. Chaque appel est donc un scan complet de ~39k lignes, répété ~1,45M de fois par import. - Les quatre entités déclarent désormais leurs index en #[ORM\Index] répétés. - Nouvelle migration : index sur city (postal_code, insee_code, name) et sur import_id de rpps / rpps_address (la purge de fin d'import filtre dessus). - Suppression de rpps_index(id_rpps) et canonical_index(canonical), doublons exacts des clés UNIQUE existantes, qui ne coûtaient que des écritures. - idx_rpps_specialty_id_rpps n'est pas rétabli : il était déclaré mais n'a jamais existé en base. Les index SPATIAL restent gérés par les migrations, #[ORM\Index] ne sachant pas les exprimer. - phpstan.neon ignore attribute.nonRepeatable : faux positif de PHPStan 1.12, l'attribut étant bien déclaré IS_REPEATABLE. Migration vérifiée aller-retour sur un MySQL 8 chargé avec le DDL de production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e le membre par nom RPPS_URL pointait sur un nom de fichier daté (PS_LibreAcces_202406110900.zip), donc chaque exécution réimportait l'extraction de juin 2024. L'endpoint stable existe et suit la même forme que CPS_URL déjà utilisée. Vérifié : il sert PS_LibreAcces_202608121208.zip (223 010 931 octets). Le fichier a d'ailleurs changé de forme depuis 2024 (56 champs au lieu de 57, 2 273 405 lignes au lieu de 2 024 964) : l'ancien se terminait par un `|` produisant un champ vide supplémentaire. Toutes les colonnes réelles sont inchangées et dans le même ordre, les index codés en dur restent donc valides. - FileProcessor::getZipMember() sélectionne le membre par expression régulière plutôt que par position. L'ordre des entrées d'une archive est décidé par le producteur : un membre ajouté ou déplacé faisait silencieusement lire un autre fichier, et tous les index de colonnes codés en dur pointaient sur autre chose. - Seul le membre correspondant est extrait, ce qui évite de décompresser les ~330 Mo de fichiers voisins que rien ne lit. - Le téléchargement accepte un sha256 attendu : une copie locale n'est réutilisée que si elle correspond, au lieu de l'être sur la seule existence du chemin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a perte du dernier lot L'import exécutait ~5 requêtes par ligne pour des données de référence qui tiennent en mémoire, et perdait silencieusement la fin de chaque exécution. Requêtes supprimées : - findCityEntity() faisait un `City::findBy(['postalCode' => ...])` par ligne d'adresse (~1,45M par génération). La table city (~39k lignes) est désormais chargée une fois en mémoire, et la cascade de désambiguïsation travaille sur des tableaux dont les noms sont normalisés au chargement plutôt qu'à chaque ligne. - findSpecialtyEntity() rechargeait l'entité à chaque ligne (~1,7M) alors qu'il n'existe que 129 libellés distincts. Le commentaire existant expliquait pourquoi : conserver une entité au travers du em->clear() de fin de lot la détache et Doctrine la refuse. On stocke donc des identifiants et on passe par getReference(), qui fournit un proxy neuf à chaque appel sans requête. - Statement / PointWrapper mémoïsent ST_GeomFromText(). rpps.coordinates et rpps_address.coordinates sont NOT NULL et l'import ne renseigne jamais de coordonnées : chaque insertion payait un aller-retour pour recalculer la constante POINT(0 0), soit ~3,1M de requêtes par cycle. Mesuré : 2000 liaisons passent de 2001 requêtes serveur à 2. - L'import utilise findOneByIdRpps() au lieu de find(), dont le OR sur trois colonnes pousse MySQL à fusionner des index et souvent à scanner. find() est inchangé, le provider d'API s'appuie dessus pour résoudre les slugs. Corrections : - Ajout du flush() final manquant : la boucle se termine entre deux lots, donc jusqu'à batchSize - 1 lignes restaient dans l'UnitOfWork et étaient perdues. Ces lignes n'étaient pas non plus estampillées, une purge de fin d'import les aurait donc supprimées. - Ordre des arguments (longitude, latitude) rétabli dans les deux chemins de liaison bruts, qui émettaient POINT(lat lng) là où PointType lit et écrit POINT(lon lat). Latent jusqu'ici car seul POINT(0 0), symétrique, est stocké. Volume de logs : une ligne était écrite par ligne sans adresse exploitable (28% des lignes) en y recopiant les 56 champs, soit ~206 Mo de sortie standard par génération. Remplacé par des compteurs affichés en fin d'exécution. La progression passe de toutes les 50 lignes à toutes les 50 000. batchSize passe de 50 à 500 : la mémoire n'est pas le facteur limitant et chaque flush est un commit de transaction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
La table des spécialités préchargée est indexée par nom exact, alors que la requête qu'elle remplace, findOneBy(['name' => ...]), s'appuyait sur la collation utf8mb4_unicode_ci de la colonne, insensible à la casse ET aux accents (vérifié sur MySQL : 'Et' = 'et' et 'é' = 'e' renvoient tous deux 1). Les noms de SpecialtyMappingService ne correspondent pas exactement à ceux en base : le mapping donne « Gastro-entérologie et hépatologie » là où la ligne vaut « Gastro-Entérologie Et Hépatologie ». La branche par nom alternatif ne trouvait donc plus rien et retombait sur la colonne texte héritée, laissant specialty_entity_id à NULL. La branche par nom alternatif utilise désormais une seconde table indexée par nom replié (ascii + minuscules), qui reproduit le comportement de la collation. La première branche reste en correspondance exacte : elle était déjà gardée par un test exact sur le tableau. Détecté par comparaison de bout en bout ancien/nouveau code sur le même fichier source : phpstan, phpcs et les 185 tests passaient tous avec le bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…surveillance Trois garde-fous nécessaires avant de laisser l'import tourner seul, indépendamment de tout découpage en tranches. 1. Visibilité des échecs. RppsImport interceptait Exception, écrivait dans error_log() et renvoyait FAILURE. L'exception étant avalée, l'écouteur console de SentryBundle ne la voyait jamais : une exécution planifiée en échec ne remontait nulle part. Le catch n'attrapait par ailleurs qu'Exception, donc une Error passait au travers. Les erreurs remontent désormais telles quelles. 2. Purge des adresses. purgeStaleAddresses() s'exécutait après *toute* exécution, y compris les invocations partielles --start-line/--limit utilisées par le planificateur : chaque tranche supprimait tout ce que les précédentes venaient d'écrire, ne laissant que les adresses de la dernière. La purge est maintenant réservée aux exécutions complètes, et refuse de s'exécuter si elle devait supprimer plus de 5% de la table (seuil réglable via --max-purge-ratio). Il s'agit d'une suppression définitive, sans historique, qui emporte les coordonnées des lignes concernées. 3. Concurrence. Verrou nommé MySQL : une seconde exécution passe son tour au lieu de s'ajouter à la première. Deux imports simultanés portent deux identifiants d'import différents, et le dernier à terminer purgerait ce que l'autre vient d'écrire. GET_LOCK est lié à la session, donc une tâche tuée libère le verrou sans délai d'expiration. Ajoute la cible import-prod-rpps-full pour l'exécution complète du planificateur ; import-prod-rpps reste disponible pour les exécutions manuelles par tranches. Vérifié sur MySQL : deux tranches successives cumulent leurs adresses (1954 puis 4269) au lieu de s'écraser ; la purge refuse 4269/4269 (100%) et laisse la table intacte ; une seconde exécution concurrente passe son tour et le verrou est bien libéré ensuite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 304510ff81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Cinq remarques de la revue automatique de la PR #60, toutes valides. Réutilisation d'une génération périmée. getZipMember() mettait l'archive en cache sous un chemin fixe (var/rpps.zip). Les endpoints servant une génération glissante, toute exécution ultérieure sur un système de fichiers persistant réutilisait indéfiniment la première archive téléchargée : le passage à l'URL stable ne suffisait donc pas à garder les données fraîches. Le cache est désormais indexé sur le nom de fichier distant, obtenu par une requête HEAD sur Content-Disposition (mesuré à 0,27 s) ; une nouvelle génération a un nom différent, donc un chemin différent. Si le nom ne peut pas être déterminé, le téléchargement est forcé plutôt que de risquer de réutiliser une archive périmée. Ce défaut était bien réel : la vérification a échoué parce que le conteneur Symfony compilé contenait encore l'ancienne RPPS_URL, ce qu'aucune exécution précédente n'avait remarqué puisqu'elles réutilisaient silencieusement l'archive locale. Archive invalide conservée. Un téléchargement interrompu, ou une réponse qui n'est pas un zip, restait en place et faisait échouer toutes les exécutions suivantes sans intervention manuelle. Le téléchargement se fait maintenant sur un fichier temporaire renommé une fois complet, et une archive illisible est supprimée avant de propager l'erreur. Déréférencement du proxy de ville. RPPSAddress::refreshOriginalAddress() passait getCity() à implode(), donc City::__toString() puis getName() : le proxy renvoyé par getReference() était chargé à chaque nouvelle adresse, rétablissant les requêtes que le préchargement des villes venait de supprimer. Le nom d'affichage est désormais transporté dans le descripteur et transmis explicitement. Verrou consultatif non portable. GET_LOCK n'existe pas sur SQLite, que le dépôt configure pour les tests et le développement : la commande échouait avant même de commencer. Le verrou n'est pris que sur MySQL. Libération du verrou masquant l'erreur d'origine. Le bloc finally exécutait une requête sur une connexion potentiellement morte ; si RELEASE_LOCK échouait à son tour, Sentry recevait l'erreur de nettoyage à la place de la cause réelle. La libération est désormais au mieux, le verrou étant de toute façon lié à la session. Sortie toujours identique octet pour octet à celle de l'ancien code sur le même fichier source, colonne original_address comprise : 9 987 lignes rpps et 7 028 rpps_address, aucune différence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Les cinq remarques de la revue automatique étaient valides, elles sont corrigées dans Réutilisation d'une génération périmée (P1). Le cache est désormais indexé sur le nom de fichier distant, obtenu par une requête HEAD sur La remarque était particulièrement bien vue : la vérification a d'abord échoué parce que le conteneur Symfony compilé contenait encore l'ancienne Archive invalide conservée (P2). Téléchargement sur fichier temporaire renommé une fois complet, et suppression d'une archive illisible avant de propager l'erreur. Déréférencement du proxy de ville (P2). Confirmé : Verrou non portable (P2) et libération masquant l'erreur d'origine (P2). Verrou pris uniquement sur MySQL ; libération au mieux, le verrou étant lié à la session. Vérification. Sortie toujours identique octet pour octet à celle de l'ancien code sur le même fichier source, colonne Le chemin complet a par ailleurs été exercé de bout en bout sur la génération courante : HEAD, téléchargement de |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a5979e15c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fuite du nom de ville entre deux lignes. findCityEntity() réinitialisait lastResolvedCityName après le retour anticipé, et non avant. Une ligne portant une voie mais ni code postal ni commune sortait donc par ce retour en conservant la valeur de la ligne précédente, valeur ensuite transmise à refreshOriginalAddress() : l'adresse se retrouvait estampillée de la commune d'un autre professionnel. La génération courante ne contient aucune ligne dans ce cas, ce qui explique que la comparaison de sortie n'ait rien montré, mais le défaut corrompt les données en silence dès qu'une telle ligne apparaît. Accumulation des générations. Indexer le cache sur le nom de fichier distant fait qu'une nouvelle génération occupe un nouveau chemin, sans que l'ancienne ne soit jamais supprimée : environ un gigaoctet par semaine sur un volume persistant. Les archives et les membres extraits appartenant à d'autres générations sont désormais supprimés après une extraction réussie. Seuls le sont les fichiers qui sont des variantes de ce qui vient d'être récupéré : archives nommées d'après cette source, et membres extraits correspondant au motif ayant servi à sélectionner l'actuel. Vérifié : après une ligne résolue sur Paris, une ligne sans code postal ni commune laisse bien le mémo à NULL ; l'élagage ramène deux archives et trois membres extraits à un seul de chacun, en laissant intacts les membres Dipl_AutExerc et SavoirFaire. Sortie inchangée par rapport à l'exécution vérifiée précédente. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Les sept remarques des deux passes de revue sont corrigées ( @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2de08893ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ZipArchive::extractTo() écrit directement à l'emplacement final. Une tâche interrompue en cours d'extraction laissait donc un fichier tronqué à ce chemin, que l'exécution suivante considérait comme extrait et importait tel quel. La conséquence n'était pas anodine : les lignes situées au-delà de la troncature ne sont jamais traitées, donc jamais estampillées de l'identifiant d'import courant, et la purge de fin d'exécution les supprime comme périmées — silencieusement dès que la part manquante reste sous le seuil de purge. Le membre est désormais écrit sur un chemin temporaire puis renommé une fois complet, rename() étant atomique au sein d'un système de fichiers. La taille est de plus comparée à celle annoncée par l'archive, à l'extraction comme à la réutilisation d'un membre déjà présent : un fichier tronqué est détecté et réextrait au lieu d'être considéré comme valide. Vérifié en tronquant volontairement le membre extrait à 50 Mo sur 815 : l'exécution suivante le détecte, le réextrait aux 815 335 679 octets attendus et lit bien les 2 273 826 lignes du fichier complet, sans laisser de fichier .part. Sortie inchangée par rapport à l'exécution vérifiée précédente. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Huitième remarque corrigée dans @codex review |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ion interrompue extractMember() nettoie son fichier temporaire en cas de taille inattendue ou d'exception, mais une tâche tuée par le système n'exécute aucun nettoyage. Le motif de sélection des membres étant ancré sur « .txt », l'élagage ne pouvait pas non plus les rattraper : un orphelin de l'ordre de 800 Mo restait donc indéfiniment, et s'en ajoutait un à chaque génération concernée. L'élagage retire désormais aussi les fichiers .part dont le nom, une fois le suffixe retiré, correspond au motif du membre courant. Vérifié : deux .part de générations antérieures sont supprimés, le membre courant et les membres Dipl_AutExerc et SavoirFaire restant intacts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Ajout de Vérifié : deux @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df025dc3d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Suppression d'index non gardée dans la migration. Sur un schéma construit par doctrine:schema:update sous ORM 3, rpps_index et canonical_index n'existent pas : la déclaration #[ORM\Table(indexes:)] qui les nommait était ignorée, et aucune migration ne les crée. Le DROP INDEX échouait donc après l'exécution des CREATE INDEX précédents, et MySQL n'annulant pas le DDL comme un tout, une nouvelle tentative entrait en collision avec les index déjà créés. Les suppressions sont désormais conditionnées à la présence de l'index, comme l'était déjà la table rpps_address. Vérifié : sur un schéma sans ces index, la migration passait de l'erreur 1091 (Can't DROP 'rpps_index') à une exécution réussie ; sur un schéma de forme production, les deux doublons sont bien supprimés et l'index SPATIAL conservé. Index SPATIAL retiré des métadonnées. Ne pas déclarer idx_coordinates ne confie pas sa gestion à la migration : l'index devient au contraire un index surnuméraire aux yeux du comparateur. Vérifié, et le résultat est net : sans la déclaration, doctrine:schema:update --complete émet « DROP INDEX idx_coordinates ON rpps » ; avec la déclaration et son drapeau spatial, plus aucune instruction ne le concerne. La recherche par proximité serait sinon retombée en balayage complet. city.coordinates n'est délibérément pas déclaré : contrairement à rpps, la production ne porte aucun index spatial sur cette colonne, et le déclarer ferait émettre une création — une modification de schéma qui relève d'une décision propre. Archives de repli non élaguées. Lorsque la requête HEAD échoue, le téléchargement utilise var/<nom>.zip et son fichier temporaire, chemins qui échappaient au filtre « <nom>-* » de l'élagage. Un repli tué en cours de route laissait donc un orphelin de la taille d'une archive. Ces deux chemins sont désormais inclus dans le balayage, sauf s'ils correspondent à l'archive courante. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Les trois remarques de la quatrième passe sont corrigées dans @codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Contexte
L'import RPPS ne produit plus de données exploitables :
rpps_addressest vide en staging (0 ligne) alors que le fichier source implique ~1,4M d'adresses, et les dernières écritures surrppsdatent de novembre 2024.Cette PR corrige les causes identifiées et divise le temps d'exécution par ~18.
Ce qui ne marchait pas
La source était figée.
RPPS_URLpointait sur un nom de fichier daté (PS_LibreAcces_202406110900.zip) : chaque exécution réimportait l'extraction de juin 2024. L'endpoint stable existe et suit la même forme queCPS_URLdéjà utilisée. Depuis cette machine, l'ancien hôte ne répond plus du tout alors que le nouveau fonctionne — à confirmer côté CloudWatch, mais l'URL figée est de toute façon à corriger.Tous les index déclarés via
#[ORM\Table(indexes:)]étaient inertes. Doctrine ORM 3 a supprimé cet argument et l'ignore silencieusement. Conséquence mesurée en production :cityn'a aucun index surpostal_code, alors que l'import exécutait unSELECT ... FROM city WHERE postal_code = ?par ligne d'adresse. Quatre entités étaient concernées (City,RPPS,RPPSAddress,Specialty) ;CCAMétait la seule à utiliser la forme supportée.La purge détruisait le travail des exécutions précédentes.
purgeStaleAddresses()s'exécutait après toute exécution, y compris les tranches--start-line/--limitutilisées par le planificateur, et chaque exécution génère un identifiant d'import différent.Les échecs étaient invisibles. La commande interceptait
Exception, écrivait danserror_log()et renvoyaitFAILURE: l'écouteur console de Sentry ne voyait jamais rien. Elle n'attrapait par ailleurs pasError.Performance
Requêtes supprimées : villes et spécialités préchargées en mémoire (~39k villes, 129 libellés) et associées via
getReference();findOneByIdRpps()au lieu duORsur trois colonnes ; mémoïsation deST_GeomFromText(), qui coûtait un aller-retour par ligne insérée pour recalculer la constantePOINT(0 0).Corrige aussi le
flush()final manquant : la boucle se terminant entre deux lots, jusqu'àbatchSize - 1lignes étaient perdues à chaque exécution.Mesures
MySQL 8, données de référence complètes, même fichier source, même limite de 10 000 lignes, rien d'autre en cours :
Sortie identique octet pour octet : 9 987 lignes
rppset 7 028rpps_addressdes deux côtés, aucune différence sur les 16 colonnes comparées derppsni les 6 derpps_address. Rattachement des spécialités 9 987 = 9 987, des villes 4 720 = 4 720.Extrapolé à la génération courante (3 300 754 lignes) : ~28 h avant, ~1 h 30 après.
Ce comparatif a d'ailleurs rattrapé une régression que phpstan, phpcs et les 185 tests laissaient passer : la table préchargée est indexée par nom exact, alors que le
findOneBy()remplacé s'appuyait sur une collation insensible à la casse et aux accents (19cffd7).Garde-fous ajoutés
--max-purge-ratiopour outrepasser).Vérifié sur MySQL : deux tranches successives cumulent leurs adresses (1954 puis 4269) au lieu de s'écraser ; la purge refuse 4269/4269 et laisse la table intacte ; une seconde exécution concurrente passe son tour.
Migration
Version20260813090000: index surcity (postal_code, insee_code, name)et surimport_idderpps/rpps_address; suppression derpps_indexetcanonical_index, doublons exacts des clés UNIQUE. Vérifiée aller-retour sur un MySQL 8 chargé avec le DDL de production.À savoir avant déploiement
import-prod-rpps-fulln'existe pas dans l'image actuellement déployée. Déployer cette PR d'abord.rpps_addressétant vide. C'est le comportement attendu : la première passe doit être lancée manuellement avec--max-purge-ratiorelevé, pour constater les volumes avant de les valider.🤖 Generated with Claude Code