Postgres persistance + job queue + api with sse #9

Merged
qpismont merged 8 commits from 1.3 into main 2026-10-26 21:19:32 +01:00
Owner

Postgres persistance + job queue + api with sse

Postgres persistance + job queue + api with sse
qpismont added 1 commit 2026-10-05 22:45:16 +02:00
instead channel and semaphore queue + upgrade to rust 1.99
qpismont added 1 commit 2026-10-06 21:41:27 +02:00
Remove apalis + redis => use postgresql instead
ci/woodpecker/push/tests Pipeline was successful
8c763786c1
qpismont changed title from Project website + use Apalis crate for task queue (with redis backend) to Postgres persistance + job queue + api with sse 2026-10-13 11:41:53 +02:00
Herald left a comment
Collaborator

Review Feedback

6 issues found.

  • 5 bug
  • 1 performance

Summary

La persistance PostgreSQL, les migrations versionnées, la limite du corps webhook et le bornage de la concurrence sont de bonnes évolutions, et la validation des positions de review ainsi que le nettoyage des sandboxes améliorent nettement la robustesse. Les points à corriger concernent surtout la réservation concurrente des jobs, l'accumulation sans limite, l'arrêt avec une concurrence nulle et la perte définitive des reviews en échec.


Cost: $0.06907231999999999

## Review Feedback ### 6 issues found. - 5 bug - 1 performance --- ### Summary La persistance PostgreSQL, les migrations versionnées, la limite du corps webhook et le bornage de la concurrence sont de bonnes évolutions, et la validation des positions de review ainsi que le nettoyage des sandboxes améliorent nettement la robustesse. Les points à corriger concernent surtout la réservation concurrente des jobs, l'accumulation sans limite, l'arrêt avec une concurrence nulle et la perte définitive des reviews en échec. --- ### Cost: $0.06907231999999999
@@ -182,3 +188,1 @@
info!(queued = rx.len(), active = tasks.len(), "Webhook received");
let permit = sem.clone().acquire_owned().await?;
let self_clone = self.clone();
let permit = semaphore.clone().acquire_owned().await?;
Collaborator

[bug] BOT_MAX_CONCURRENT=0 est accepté par le parsing de configuration, mais acquire_owned() attendra alors indéfiniment un permis. Comme cette attente n'est pas incluse dans un select! avec l'annulation, l'arrêt peut rester bloqué jusqu'au délai forcé de 60 secondes. Il faut rejeter zéro au chargement de configuration ou rendre l'attente annulable.

**[bug]** `BOT_MAX_CONCURRENT=0` est accepté par le parsing de configuration, mais `acquire_owned()` attendra alors indéfiniment un permis. Comme cette attente n'est pas incluse dans un `select!` avec l'annulation, l'arrêt peut rester bloqué jusqu'au délai forcé de 60 secondes. Il faut rejeter zéro au chargement de configuration ou rendre l'attente annulable.
qpismont marked this conversation as resolved
@@ -184,1 +188,3 @@
let self_clone = self.clone();
let permit = semaphore.clone().acquire_owned().await?;
if let Err(err) = self.queue.mark_processing(job.id).await {
Collaborator

[bug] En cas d'échec de mark_processing, le code journalise l'erreur mais lance quand même le traitement. Le job reste alors Pending et peut être repris par le poller pendant que la review est déjà en cours, ce qui crée des traitements et commentaires en double. Il faut abandonner ce job (ou le remettre explicitement dans un état contrôlé) lorsque la réservation échoue.

**[bug]** En cas d'échec de `mark_processing`, le code journalise l'erreur mais lance quand même le traitement. Le job reste alors `Pending` et peut être repris par le poller pendant que la review est déjà en cours, ce qui crée des traitements et commentaires en double. Il faut abandonner ce job (ou le remettre explicitement dans un état contrôlé) lorsque la réservation échoue.
qpismont marked this conversation as resolved
@@ -204,0 +255,4 @@
duration_ms,
};
if let Err(err) = self.dedup.mark_reviewed(record).await {
Collaborator

[bug] Même quand exec échoue, le code enregistre une Review avec ok = false, puis supprime le job. Un webhook de retry sera ensuite considéré comme déjà traité par is_reviewed et ignoré : une panne temporaire de Gitea, du sandbox ou d'OpenRouter devient donc un échec définitif sans possibilité de retry. Les jobs échoués devraient rester en file (avec backoff/retry) ou être distingués des reviews effectivement publiées.

**[bug]** Même quand `exec` échoue, le code enregistre une `Review` avec `ok = false`, puis supprime le job. Un webhook de retry sera ensuite considéré comme déjà traité par `is_reviewed` et ignoré : une panne temporaire de Gitea, du sandbox ou d'OpenRouter devient donc un échec définitif sans possibilité de retry. Les jobs échoués devraient rester en file (avec backoff/retry) ou être distingués des reviews effectivement publiées.
qpismont marked this conversation as resolved
@@ -0,0 +12,4 @@
pub async fn connect(url: &str) -> toasty::Result<Db> {
let mut db = open(url).await?;
Job::update_by_status(JobStatus::Processing)
Collaborator

[bug] Au démarrage, tous les jobs Processing sont remis à Pending sans notion de propriétaire ou de lease. Si deux instances tournent simultanément, le démarrage de l'une peut réenfiler les jobs activement traités par l'autre, en plus de la course déjà présente dans le poller. La récupération après crash doit être limitée aux jobs dont le lease a expiré, et le traitement doit être atomiquement réservé.

**[bug]** Au démarrage, tous les jobs `Processing` sont remis à `Pending` sans notion de propriétaire ou de lease. Si deux instances tournent simultanément, le démarrage de l'une peut réenfiler les jobs activement traités par l'autre, en plus de la course déjà présente dans le poller. La récupération après crash doit être limitée aux jobs dont le lease a expiré, et le traitement doit être atomiquement réservé.
qpismont marked this conversation as resolved
@@ -0,0 +65,4 @@
return Some((job, buffer));
}
match Job::filter_by_status(JobStatus::Pending)
Collaborator

[bug] La lecture des jobs Pending et leur passage à Processing ne sont pas atomiques. Avec plusieurs instances du serveur (ou plusieurs consommateurs), elles peuvent toutes récupérer le même job avant l'appel à mark_processing, puis exécuter plusieurs fois la review et publier des commentaires dupliqués. Il faut effectuer une réservation conditionnelle en base (UPDATE ... WHERE status = 'pending' ... RETURNING) plutôt que seulement filtrer puis mettre à jour.

**[bug]** La lecture des jobs `Pending` et leur passage à `Processing` ne sont pas atomiques. Avec plusieurs instances du serveur (ou plusieurs consommateurs), elles peuvent toutes récupérer le même job avant l'appel à `mark_processing`, puis exécuter plusieurs fois la review et publier des commentaires dupliqués. Il faut effectuer une réservation conditionnelle en base (`UPDATE ... WHERE status = 'pending' ... RETURNING`) plutôt que seulement filtrer puis mettre à jour.
Collaborator

[performance] Cette requête récupère tous les jobs en attente sans limite, puis les accumule dans le VecDeque à la ligne suivante. Une file PostgreSQL qui reçoit plus de jobs qu'elle n'en traite peut donc faire croître la mémoire du processus sans borne pratique. Il faudrait paginer ou limiter le nombre de jobs réservés par poll.

**[performance]** Cette requête récupère tous les jobs en attente sans limite, puis les accumule dans le `VecDeque` à la ligne suivante. Une file PostgreSQL qui reçoit plus de jobs qu'elle n'en traite peut donc faire croître la mémoire du processus sans borne pratique. Il faudrait paginer ou limiter le nombre de jobs réservés par poll.
qpismont marked this conversation as resolved
Herald left a comment
Collaborator

Review Feedback

3 issues found.

  • 2 bug
  • 1 performance

Summary

La file PostgreSQL apporte une bonne base pour la reprise après redémarrage, et les erreurs d’enqueue sont correctement renvoyées à l’appelant. À corriger : le chargement non borné des jobs en attente, la poursuite du traitement après échec du marquage, et la suppression du job lorsque l’enregistrement du résultat échoue.


Cost: $0.022133020000000003

## Review Feedback ### 3 issues found. - 2 bug - 1 performance --- ### Summary La file PostgreSQL apporte une bonne base pour la reprise après redémarrage, et les erreurs d’enqueue sont correctement renvoyées à l’appelant. À corriger : le chargement non borné des jobs en attente, la poursuite du traitement après échec du marquage, et la suppression du job lorsque l’enregistrement du résultat échoue. --- ### Cost: $0.022133020000000003
@@ -184,1 +188,3 @@
let self_clone = self.clone();
let permit = semaphore.clone().acquire_owned().await?;
if let Err(err) = self.queue.mark_processing(job.id).await {
Collaborator

[bug] Si la mise à jour du statut échoue, le job est quand même traité. Il reste alors Pending et sera récupéré au prochain sondage, ce qui peut lancer plusieurs reviews concurrentes pour le même événement. Ne lancez pas le traitement si cette transition échoue, ou garantissez une prise en charge atomique.

**[bug]** Si la mise à jour du statut échoue, le job est quand même traité. Il reste alors `Pending` et sera récupéré au prochain sondage, ce qui peut lancer plusieurs reviews concurrentes pour le même événement. Ne lancez pas le traitement si cette transition échoue, ou garantissez une prise en charge atomique.
qpismont marked this conversation as resolved
@@ -204,0 +259,4 @@
error!(%err, "failed to record review");
}
if let Err(err) = self.queue.remove(job.id).await {
Collaborator

[bug] Si mark_reviewed échoue, l’erreur est seulement journalisée, puis le job est supprimé. Une panne temporaire de la base peut donc faire perdre définitivement l’événement sans enregistrer son résultat. Ne supprimez le job qu’après l’enregistrement réussi, ou conservez-le pour reprise.

**[bug]** Si `mark_reviewed` échoue, l’erreur est seulement journalisée, puis le job est supprimé. Une panne temporaire de la base peut donc faire perdre définitivement l’événement sans enregistrer son résultat. Ne supprimez le job qu’après l’enregistrement réussi, ou conservez-le pour reprise.
qpismont marked this conversation as resolved
@@ -0,0 +65,4 @@
return Some((job, buffer));
}
match Job::filter_by_status(JobStatus::Pending)
Collaborator

[performance] Cette requête récupère tous les jobs en attente, puis les met en mémoire dans le buffer. Si la file grossit ou si le service redémarre avec un grand retard à rattraper, chaque worker peut charger un volume non borné de payloads JSON. Limitez la taille des lots et ne chargez que les jobs nécessaires à la capacité disponible.

**[performance]** Cette requête récupère tous les jobs en attente, puis les met en mémoire dans le buffer. Si la file grossit ou si le service redémarre avec un grand retard à rattraper, chaque worker peut charger un volume non borné de payloads JSON. Limitez la taille des lots et ne chargez que les jobs nécessaires à la capacité disponible.
qpismont marked this conversation as resolved
Herald left a comment
Collaborator

Review Feedback

4 issues found.

  • 4 bug

Summary

La PR apporte une base solide pour la persistance PostgreSQL, une file durable, des schémas explicites et des tests d’intégration, et la documentation comme les métriques sont bien étoffées. Les points à corriger concernent le traitement concurrent des jobs, leur récupération au démarrage, l’arrêt gracieux et les instructions de mise en route.


Cost: $0.021829394999999998

## Review Feedback ### 4 issues found. - 4 bug --- ### Summary La PR apporte une base solide pour la persistance PostgreSQL, une file durable, des schémas explicites et des tests d’intégration, et la documentation comme les métriques sont bien étoffées. Les points à corriger concernent le traitement concurrent des jobs, leur récupération au démarrage, l’arrêt gracieux et les instructions de mise en route. --- ### Cost: $0.021829394999999998
@@ -182,3 +188,1 @@
info!(queued = rx.len(), active = tasks.len(), "Webhook received");
let permit = sem.clone().acquire_owned().await?;
let self_clone = self.clone();
let permit = semaphore.clone().acquire_owned().await?;
Collaborator

[bug] L’attente d’un permis n’écoute pas shutdown. Si tous les slots sont occupés au moment de l’arrêt, le bot attend qu’un slot se libère puis lance quand même le job en attente, et peut continuer ainsi à vider la file. Cela contredit le comportement annoncé où seuls les jobs déjà en cours terminent ; rendez aussi cette attente interruptible.

**[bug]** L’attente d’un permis n’écoute pas `shutdown`. Si tous les slots sont occupés au moment de l’arrêt, le bot attend qu’un slot se libère puis lance quand même le job en attente, et peut continuer ainsi à vider la file. Cela contredit le comportement annoncé où seuls les jobs déjà en cours terminent ; rendez aussi cette attente interruptible.
qpismont marked this conversation as resolved
@@ -0,0 +12,4 @@
pub async fn connect(url: &str) -> toasty::Result<Db> {
let mut db = open(url).await?;
Job::update_by_status(JobStatus::Processing)
Collaborator

[bug] Au démarrage, chaque instance remet à Pending tous les jobs Processing, y compris ceux qu’une autre instance est encore en train de traiter. Un redémarrage ou déploiement pendant une review peut donc la faire exécuter en double. La récupération doit distinguer les jobs réellement abandonnés de ceux détenus par des workers actifs.

**[bug]** Au démarrage, chaque instance remet à `Pending` tous les jobs `Processing`, y compris ceux qu’une autre instance est encore en train de traiter. Un redémarrage ou déploiement pendant une review peut donc la faire exécuter en double. La récupération doit distinguer les jobs réellement abandonnés de ceux détenus par des workers actifs.
qpismont marked this conversation as resolved
@@ -0,0 +65,4 @@
return Some((job, buffer));
}
match Job::filter_by_status(JobStatus::Pending)
Collaborator

[bug] La lecture des jobs en attente n’en réserve aucun : deux instances peuvent récupérer le même job avant que l’une d’elles n’appelle mark_processing, puis lancer deux reviews et poster des résultats en double. Il faut réclamer les jobs de façon atomique, par exemple avec une mise à jour conditionnelle ou un verrouillage SKIP LOCKED.

**[bug]** La lecture des jobs en attente n’en réserve aucun : deux instances peuvent récupérer le même job avant que l’une d’elles n’appelle `mark_processing`, puis lancer deux reviews et poster des résultats en double. Il faut réclamer les jobs de façon atomique, par exemple avec une mise à jour conditionnelle ou un verrouillage `SKIP LOCKED`.
qpismont marked this conversation as resolved
@@ -0,0 +551,4 @@
<strong>Configure every required setting</strong>
<pre>cp .env.example .env
$EDITOR .env</pre>
<p>Set all nine required variables:</p>
Collaborator

[bug] Le guide demande de configurer neuf variables, mais omet DATABASE_URL, désormais obligatoire, puis passe directement au lancement sans expliquer comment appliquer les migrations. Un nouvel utilisateur suivant ces étapes ne peut pas démarrer le service. Ajoutez la configuration de PostgreSQL et l’étape migration apply.

**[bug]** Le guide demande de configurer neuf variables, mais omet `DATABASE_URL`, désormais obligatoire, puis passe directement au lancement sans expliquer comment appliquer les migrations. Un nouvel utilisateur suivant ces étapes ne peut pas démarrer le service. Ajoutez la configuration de PostgreSQL et l’étape `migration apply`.
qpismont marked this conversation as resolved
qpismont added 1 commit 2026-10-13 18:14:52 +02:00
Add live dashboard with SSE job events
ci/woodpecker/push/tests Pipeline was successful
7493de6952
Expose /jobs, /dedup, and /events routes backed by a new EventBus that
broadcasts job lifecycle and per-job progress over SSE. Serve a static
dashboard from front/ that lists pending and in-progress jobs, streams
agent activity, and shows completed reviews.

Also add migrate-apply/migrate-generate scripts that load .env, update
the Containerfile to ship the front assets and use the apply script, and
refresh the roadmap.
Herald left a comment
Collaborator

Review Feedback

7 issues found.

  • 5 bug
  • 1 security
  • 1 performance

Summary

La PR apporte une vraie file durable en PostgreSQL, des événements SSE avec historique de progression, et un dashboard utile pour suivre les jobs. Les principaux points à corriger concernent l’exposition sans authentification des données de review, la réservation non atomique des jobs et plusieurs cas de concurrence ou de reconnexion qui peuvent produire des traitements en double ou désynchroniser l’interface.


Cost: $0.030983235

## Review Feedback ### 7 issues found. - 5 bug - 1 security - 1 performance --- ### Summary La PR apporte une vraie file durable en PostgreSQL, des événements SSE avec historique de progression, et un dashboard utile pour suivre les jobs. Les principaux points à corriger concernent l’exposition sans authentification des données de review, la réservation non atomique des jobs et plusieurs cas de concurrence ou de reconnexion qui peuvent produire des traitements en double ou désynchroniser l’interface. --- ### Cost: $0.030983235
@@ -27,2 +40,3 @@
.route("/", get(root))
.route("/webhook", post(webhook))
.route("/dedup", get(dedup_list))
.route("/jobs", get(jobs_list))
Collaborator

[security] Ces routes de consultation et les flux SSE sont accessibles sans authentification. Elles exposent notamment les noms de dépôts, les reviews et les sorties des outils exécutés sur le code, qui peuvent contenir des données de dépôts privés. Protégez ces endpoints ou limitez explicitement leur accès.

**[security]** Ces routes de consultation et les flux SSE sont accessibles sans authentification. Elles exposent notamment les noms de dépôts, les reviews et les sorties des outils exécutés sur le code, qui peuvent contenir des données de dépôts privés. Protégez ces endpoints ou limitez explicitement leur accès.
qpismont marked this conversation as resolved
@@ -64,3 +146,1 @@
if app_state.bot.check_and_mark(event_id).await {
metrics::webhook_duplicate(event_type);
return Err(AppError::AlreadyProcessedErr);
match app_state.dedup.is_known(event_id).await {
Collaborator

[bug] Le test is_known et l’insertion dans la file ne sont pas atomiques. Deux livraisons simultanées du même webhook peuvent toutes deux passer le test ; la contrainte unique fait ensuite échouer l’une et renvoie 503 au lieu de traiter ce doublon comme déjà accepté. Gérez explicitement ce conflit d’insertion.

**[bug]** Le test `is_known` et l’insertion dans la file ne sont pas atomiques. Deux livraisons simultanées du même webhook peuvent toutes deux passer le test ; la contrainte unique fait ensuite échouer l’une et renvoie 503 au lieu de traiter ce doublon comme déjà accepté. Gérez explicitement ce conflit d’insertion.
qpismont marked this conversation as resolved
@@ -184,1 +193,3 @@
let self_clone = self.clone();
let permit = semaphore.clone().acquire_owned().await?;
if let Err(err) = self.queue.mark_processing(job.id).await {
Collaborator

[bug] En cas d’échec de mark_processing, le job est tout de même traité. Il reste alors Pending en base et pourra être repris après redémarrage — ou par une autre instance — pendant que cette exécution peut déjà publier la review. N’exécutez pas le job si sa transition d’état échoue.

**[bug]** En cas d’échec de `mark_processing`, le job est tout de même traité. Il reste alors Pending en base et pourra être repris après redémarrage — ou par une autre instance — pendant que cette exécution peut déjà publier la review. N’exécutez pas le job si sa transition d’état échoue.
qpismont marked this conversation as resolved
@@ -0,0 +73,4 @@
pub async fn list_reviews(&self) -> toasty::Result<Vec<Review>> {
let mut db = self.db.clone();
let reviews = Review::all().exec(&mut db).await?;
Collaborator

[performance] Review::all() charge toutes les reviews et leurs champs, notamment le Markdown, à chaque rafraîchissement du dashboard. Cette requête et la réponse JSON grossissent sans limite avec l’historique. Ajoutez une pagination ou une limite et ne chargez que les champs nécessaires à la liste.

**[performance]** `Review::all()` charge toutes les reviews et leurs champs, notamment le Markdown, à chaque rafraîchissement du dashboard. Cette requête et la réponse JSON grossissent sans limite avec l’historique. Ajoutez une pagination ou une limite et ne chargez que les champs nécessaires à la liste.
qpismont marked this conversation as resolved
@@ -0,0 +57,4 @@
return Some((job, buffer));
}
match Job::filter_by_status(JobStatus::Pending)
Collaborator

[bug] La sélection des jobs Pending et leur passage à Processing sont deux opérations distinctes, sans réservation atomique. Avec plusieurs instances, chacune peut récupérer le même job et lancer la même review avant que l’autre ne le marque Processing. Il faut réclamer les jobs de façon atomique.

**[bug]** La sélection des jobs Pending et leur passage à Processing sont deux opérations distinctes, sans réservation atomique. Avec plusieurs instances, chacune peut récupérer le même job et lancer la même review avant que l’autre ne le marque Processing. Il faut réclamer les jobs de façon atomique.
qpismont marked this conversation as resolved
@@ -0,0 +23,4 @@
this._selectedId = null;
this.render();
this.refresh();
Collaborator

[bug] refresh() est lancé sans attendre avant l’abonnement SSE. Si un événement arrive pendant le chargement, la réponse REST peut ensuite écraser l’état plus récent fourni par SSE ; aucun nouvel événement n’est garanti pour corriger la liste. Abonnez-vous avant de charger l’état, ou réconciliez la réponse avec les événements reçus entre-temps.

**[bug]** `refresh()` est lancé sans attendre avant l’abonnement SSE. Si un événement arrive pendant le chargement, la réponse REST peut ensuite écraser l’état plus récent fourni par SSE ; aucun nouvel événement n’est garanti pour corriger la liste. Abonnez-vous avant de charger l’état, ou réconciliez la réponse avec les événements reçus entre-temps.
qpismont marked this conversation as resolved
@@ -0,0 +52,4 @@
// Un `error` survient aussi quand le serveur ferme le flux (job terminé).
// On ferme alors la source pour éviter la boucle de reconnexion (le job
// n'est plus en cours, la route renverrait 404).
onError: () => {
Collaborator

[bug] Le gestionnaire ferme l’EventSource à la première erreur, y compris lors d’une coupure réseau temporaire pour laquelle EventSource aurait normalement réessayé. L’utilisateur perd alors les mises à jour du job. Fermez-le seulement quand la fin du job est confirmée ou quand le serveur indique que le job n’existe plus.

**[bug]** Le gestionnaire ferme l’EventSource à la première erreur, y compris lors d’une coupure réseau temporaire pour laquelle EventSource aurait normalement réessayé. L’utilisateur perd alors les mises à jour du job. Fermez-le seulement quand la fin du job est confirmée ou quand le serveur indique que le job n’existe plus.
qpismont marked this conversation as resolved
qpismont added 1 commit 2026-10-13 21:57:38 +02:00
clean project structure
ci/woodpecker/push/tests Pipeline was canceled
3078ba5705
Herald left a comment
Collaborator

Review Feedback

9 issues found.

  • 4 bug
  • 1 security
  • 3 performance
  • 1 maintainability

Summary

Cette PR apporte une persistance Postgres propre avec migrations Toasty, une file de jobs durable et un dashboard SSE. Elle est bien structurée et bien testée.

Ce qui est bien fait :

  • Le claim_next atomique (UPDATE avec sous-requête et RETURNING).
  • Le requeue_interrupted au démarrage.
  • Le réveil du worker par Notify plutôt que par polling.
  • Le sémaphore pris avant le claim.
  • Le bus d'événements avec historique rejoué et abonnement sans trou.
  • Le délai maximal d'arrêt, avec fermeture des flux SSE.
  • Le passage à rustls/ring, verrouillé par cargo deny.
  • Le découpage du module review en sous-modules.
  • L'échappement HTML côté front.
  • La documentation des migrations et de la file de jobs.

Problèmes à traiter :

  • Sécurité : les routes du dashboard et les flux SSE sont publics sans authentification. Ils exposent des dépôts privés, des payloads et des extraits de code.
  • Webhooks : la course entre is_known et push provoque des 503 au lieu d'un 200 « déjà traité ».
  • Ressources : /reviews n'est pas paginé, et les sorties d'outils conservées dans l'historique SSE ne sont pas tronquées.
  • Robustesse du worker : une panique de tâche laisse le job bloqué en processing et fait dériver la jauge.
  • Front : le message « vide » reste affiché dans job-list, il n'y a pas de resynchronisation à la reconnexion SSE, et le bouton « Watch » mène à un 404 pour les jobs en attente.
  • Outillage : le socket podman est codé en dur dans le devcontainer, cargo-deny est recompilé à chaque run de CI, et les versions de Rust diffèrent entre le Containerfile et la CI.

Cost: $1.4524780000000002

## Review Feedback ### 9 issues found. - 4 bug - 1 security - 3 performance - 1 maintainability --- ### Summary Cette PR apporte une persistance Postgres propre avec migrations Toasty, une file de jobs durable et un dashboard SSE. Elle est bien structurée et bien testée. Ce qui est bien fait : - Le `claim_next` atomique (UPDATE avec sous-requête et RETURNING). - Le `requeue_interrupted` au démarrage. - Le réveil du worker par `Notify` plutôt que par polling. - Le sémaphore pris avant le claim. - Le bus d'événements avec historique rejoué et abonnement sans trou. - Le délai maximal d'arrêt, avec fermeture des flux SSE. - Le passage à rustls/ring, verrouillé par `cargo deny`. - Le découpage du module `review` en sous-modules. - L'échappement HTML côté front. - La documentation des migrations et de la file de jobs. Problèmes à traiter : - **Sécurité :** les routes du dashboard et les flux SSE sont publics sans authentification. Ils exposent des dépôts privés, des payloads et des extraits de code. - **Webhooks :** la course entre `is_known` et `push` provoque des 503 au lieu d'un 200 « déjà traité ». - **Ressources :** `/reviews` n'est pas paginé, et les sorties d'outils conservées dans l'historique SSE ne sont pas tronquées. - **Robustesse du worker :** une panique de tâche laisse le job bloqué en `processing` et fait dériver la jauge. - **Front :** le message « vide » reste affiché dans `job-list`, il n'y a pas de resynchronisation à la reconnexion SSE, et le bouton « Watch » mène à un 404 pour les jobs en attente. - **Outillage :** le socket podman est codé en dur dans le devcontainer, `cargo-deny` est recompilé à chaque run de CI, et les versions de Rust diffèrent entre le `Containerfile` et la CI. --- ### Cost: $1.4524780000000002
@@ -20,2 +20,4 @@
"workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/steep,type=bind",
"workspaceFolder": "/workspaces/steep",
"mounts": [
"source=/run/user/1000/podman/podman.sock,target=/var/run/docker.sock,type=bind"
Collaborator

[maintainability] Le socket podman est monté avec le chemin en dur /run/user/1000/podman/podman.sock. Le devcontainer ne démarre pas pour un utilisateur dont l'UID n'est pas 1000, alors que le build utilise ${localEnv:UID}. De plus, si le socket n'existe pas, le montage fait échouer l'ouverture du conteneur. Il faut utiliser ${localEnv:XDG_RUNTIME_DIR}/podman/podman.sock.

**[maintainability]** Le socket podman est monté avec le chemin en dur `/run/user/1000/podman/podman.sock`. Le devcontainer ne démarre pas pour un utilisateur dont l'UID n'est pas 1000, alors que le build utilise `${localEnv:UID}`. De plus, si le socket n'existe pas, le montage fait échouer l'ouverture du conteneur. Il faut utiliser `${localEnv:XDG_RUNTIME_DIR}/podman/podman.sock`.
@@ -20,2 +27,3 @@
image: rust:1.99
commands:
- cargo test --workspace --all-targets
- cargo install cargo-deny --locked --version 0.18.9
Collaborator

[performance] cargo install cargo-deny --locked recompile l'outil à chaque pipeline, ce qui ajoute plusieurs minutes de CI. Il vaut mieux utiliser une image préconstruite (ghcr.io/embarkstudios/cargo-deny) ou un binaire précompilé. Par ailleurs les étapes CI utilisent rust:1.99 alors que le Containerfile build avec rust:1.98-trixie : les versions de toolchain divergent.

**[performance]** `cargo install cargo-deny --locked` recompile l'outil à chaque pipeline, ce qui ajoute plusieurs minutes de CI. Il vaut mieux utiliser une image préconstruite (`ghcr.io/embarkstudios/cargo-deny`) ou un binaire précompilé. Par ailleurs les étapes CI utilisent `rust:1.99` alors que le `Containerfile` build avec `rust:1.98-trixie` : les versions de toolchain divergent.
@@ -26,3 +39,3 @@
let app = Router::new()
.route("/", get(root))
.route("/webhook", post(webhook))
.route("/reviews", get(reviews_list))
Collaborator

[security] Les routes /reviews, /jobs, /events et /jobs/{id}/events sont publiques, sans authentification. Elles exposent les noms de dépôts privés, les numéros de PR, le payload complet des webhooks (corps des commentaires, clone_url), le markdown des reviews et le flux des appels d'outils de l'agent. Ce flux contient des extraits du code des dépôts, ce qui est sensible. Le service tourne sur 0.0.0.0 et le dashboard est servi par le même processus. Il faut protéger ces routes (token ou basic auth, ou reverse proxy authentifié), ou au moins les placer sur un port interne distinct du /webhook. Il n'y a aussi aucune limite sur le nombre de connexions SSE ouvertes.

**[security]** Les routes `/reviews`, `/jobs`, `/events` et `/jobs/{id}/events` sont publiques, sans authentification. Elles exposent les noms de dépôts privés, les numéros de PR, le payload complet des webhooks (corps des commentaires, clone_url), le markdown des reviews et le flux des appels d'outils de l'agent. Ce flux contient des extraits du code des dépôts, ce qui est sensible. Le service tourne sur `0.0.0.0` et le dashboard est servi par le même processus. Il faut protéger ces routes (token ou basic auth, ou reverse proxy authentifié), ou au moins les placer sur un port interne distinct du `/webhook`. Il n'y a aussi aucune limite sur le nombre de connexions SSE ouvertes.
@@ -71,3 +158,1 @@
metrics::webhook_channel_full(event_type);
return Err(AppError::ChannelFullErr);
}
let job = match app_state.queue.push(wb).await {
Collaborator

[bug] Le contrôle is_known puis queue.push n'est pas atomique. Deux livraisons concurrentes du même événement (retry Gitea) passent toutes les deux le contrôle. La seconde échoue sur la contrainte UNIQUE(event_id) et renvoie 503 Could not enqueue task, avec un faux événement Sentry et le compteur enqueue_failed. Gitea risque alors de re-livrer en boucle. Il faut traiter la violation d'unicité comme AlreadyProcessedErr (200), ou faire un insert avec ON CONFLICT DO NOTHING.

**[bug]** Le contrôle `is_known` puis `queue.push` n'est pas atomique. Deux livraisons concurrentes du même événement (retry Gitea) passent toutes les deux le contrôle. La seconde échoue sur la contrainte `UNIQUE(event_id)` et renvoie 503 `Could not enqueue task`, avec un faux événement Sentry et le compteur `enqueue_failed`. Gitea risque alors de re-livrer en boucle. Il faut traiter la violation d'unicité comme `AlreadyProcessedErr` (200), ou faire un insert avec `ON CONFLICT DO NOTHING`.
@@ -0,0 +218,4 @@
iteration,
name: name.to_string(),
truncated,
output: output.to_string(),
Collaborator

[performance] Contrairement aux arguments (tronqués à 200 caractères), output est conservé en entier, jusqu'à 32 Ko par appel d'outil. L'historique garde 256 événements par job, soit environ 8 Mo par job actif. Chaque nouvel abonné SSE reçoit ensuite tout cet historique. Il faut tronquer l'aperçu envoyé au dashboard (comme pour MAX_EVENT_ARGUMENTS), ce qui limite aussi ce qui fuit sur une route publique.

**[performance]** Contrairement aux arguments (tronqués à 200 caractères), `output` est conservé en entier, jusqu'à 32 Ko par appel d'outil. L'historique garde 256 événements par job, soit environ 8 Mo par job actif. Chaque nouvel abonné SSE reçoit ensuite tout cet historique. Il faut tronquer l'aperçu envoyé au dashboard (comme pour `MAX_EVENT_ARGUMENTS`), ce qui limite aussi ce qui fuit sur une route publique.
@@ -0,0 +59,4 @@
pub async fn list(&self) -> toasty::Result<Vec<Review>> {
let mut db = self.db.clone();
let reviews = Review::all().exec(&mut db).await?;
Collaborator

[performance] list() charge toutes les reviews, y compris la colonne markdown, sans pagination ni tri, et /reviews les renvoie toutes d'un coup. La table ne fait que grossir et chaque ouverture ou rafraîchissement du dashboard relit tout l'historique. Il faut une limite (par exemple les N dernières, triées par completed_at DESC), ou omettre markdown dans la liste.

**[performance]** `list()` charge toutes les reviews, y compris la colonne `markdown`, sans pagination ni tri, et `/reviews` les renvoie toutes d'un coup. La table ne fait que grossir et chaque ouverture ou rafraîchissement du dashboard relit tout l'historique. Il faut une limite (par exemple les N dernières, triées par `completed_at DESC`), ou omettre `markdown` dans la liste.
@@ -0,0 +52,4 @@
this._source = subscribe(endpoints.lifecycle, {
onLifecycle: (event) => this.applyLifecycle(event),
// En cas de gap (évènements ratés), on recharge l'état depuis le REST.
onGap: () => this.refresh(),
Collaborator

[bug] Le flux /events reconnecte automatiquement (EventSource), mais les événements émis pendant la coupure (redémarrage serveur, réseau) sont perdus, sans événement gap. Le dashboard garde alors un état périmé (jobs fantômes dans Pending/In progress). Il faut brancher onOpen pour appeler refresh() à chaque (re)connexion, y compris la première.

**[bug]** Le flux `/events` reconnecte automatiquement (EventSource), mais les événements émis pendant la coupure (redémarrage serveur, réseau) sont perdus, sans événement `gap`. Le dashboard garde alors un état périmé (jobs fantômes dans Pending/In progress). Il faut brancher `onOpen` pour appeler `refresh()` à chaque (re)connexion, y compris la première.
@@ -0,0 +43,4 @@
${job.event_id != null ? `<div><dt>Event</dt><dd>${job.event_id}</dd></div>` : ""}
<div><dt>Created</dt><dd>${escapeHtml(relativeTime(job.created_at))}</dd></div>
</dl>
<button class="button button-secondary job-watch" type="button">Watch activity</button>
Collaborator

[bug] Le bouton « Watch activity » est affiché aussi pour les jobs pending. Or /jobs/{id}/events renvoie 404 tant que le job n'a pas démarré (le canal n'est créé que par EventBus::start). Le panneau affiche alors « stream closed » sans explication. Il faut masquer ou désactiver le bouton pour les jobs en attente, ou ré-ouvrir le flux au passage en started.

**[bug]** Le bouton « Watch activity » est affiché aussi pour les jobs `pending`. Or `/jobs/{id}/events` renvoie 404 tant que le job n'a pas démarré (le canal n'est créé que par `EventBus::start`). Le panneau affiche alors « stream closed » sans explication. Il faut masquer ou désactiver le bouton pour les jobs en attente, ou ré-ouvrir le flux au passage en `started`.
@@ -0,0 +51,4 @@
this.append(card);
}
for (const card of known.values()) card.remove();
Collaborator

[bug] Dans la branche « liste vide », render() ajoute un <p class="empty">, mais la branche non vide ne le retire jamais. Après la première apparition d'un job (liste vide puis job), le message « No pending job. » reste affiché au-dessus ou au-dessous des cartes. Il faut supprimer l'élément .empty dans la branche non vide, ou faire un replaceChildren avant de ré-attacher les cartes.

**[bug]** Dans la branche « liste vide », `render()` ajoute un `<p class="empty">`, mais la branche non vide ne le retire jamais. Après la première apparition d'un job (liste vide puis job), le message « No pending job. » reste affiché au-dessus ou au-dessous des cartes. Il faut supprimer l'élément `.empty` dans la branche non vide, ou faire un `replaceChildren` avant de ré-attacher les cartes.
qpismont added 1 commit 2026-10-13 22:09:23 +02:00
Fix rust version
ci/woodpecker/push/tests Pipeline was successful
131f1ec84a
qpismont added 1 commit 2026-10-14 18:42:02 +02:00
rwlock to mutex
ci/woodpecker/push/tests Pipeline failed
b40985528e
qpismont added 1 commit 2026-10-14 18:50:14 +02:00
same migration process test and prod
ci/woodpecker/push/tests Pipeline was canceled
cedd052a4d
qpismont added 1 commit 2026-10-14 18:52:17 +02:00
update lock cve
ci/woodpecker/push/tests Pipeline was successful
426a10d5be
qpismont merged commit 74207da914 into main 2026-10-26 21:19:32 +01:00
qpismont deleted branch 1.3 2026-10-26 21:19:32 +01:00
Sign in to join this conversation.
No Reviewers
No labels
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: qpismont/steep#9